Skip to content

Conversation

artagnon
Copy link
Contributor

Protect the pmpy-mod.ll test against O2 pipeline changes, by changing the opt invocation to call exactly the prerequisite passes before calling hexagon-loop-idiom. The context for this change is that a HashRecognize analysis was recently added to LLVM, and the optimization of CRC loops will soon be enabled by default, which would cause pmpy-mod.ll to fail, since the CRC loop would be optimized by a table-lookup before HexagonLoopIdiom has a chance to optimize it using pmpy instructions. HexagonLoopIdiom should probably be removed in the future, preferring the generic middle-end optimization performed by using HashRecognize.

-- 8< --
See #143208.

Protect the pmpy-mod.ll test against O2 pipeline changes, by changing
the opt invocation to call exactly the prerequisite passes before
calling hexagon-loop-idiom. The context for this change is that a
HashRecognize analysis was recently added to LLVM, and the optimization
of CRC loops will soon be enabled by default, which would cause
pmpy-mod.ll to fail, since the CRC loop would be optimized by a
table-lookup before HexagonLoopIdiom has a chance to optimize it using
pmpy instructions. HexagonLoopIdiom should probably be removed in the
future, preferring the generic middle-end optimization performed by
using HashRecognize.
@llvmbot
Copy link
Member

llvmbot commented Jun 18, 2025

@llvm/pr-subscribers-backend-hexagon

Author: Ramkumar Ramachandra (artagnon)

Changes

Protect the pmpy-mod.ll test against O2 pipeline changes, by changing the opt invocation to call exactly the prerequisite passes before calling hexagon-loop-idiom. The context for this change is that a HashRecognize analysis was recently added to LLVM, and the optimization of CRC loops will soon be enabled by default, which would cause pmpy-mod.ll to fail, since the CRC loop would be optimized by a table-lookup before HexagonLoopIdiom has a chance to optimize it using pmpy instructions. HexagonLoopIdiom should probably be removed in the future, preferring the generic middle-end optimization performed by using HashRecognize.

-- 8< --
See #143208.


Full diff: https://github.com/llvm/llvm-project/pull/144734.diff

1 Files Affected:

  • (modified) llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll (+2-6)
diff --git a/llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll b/llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll
index 836a0e110b9e0..e4ab9b131e749 100644
--- a/llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll
+++ b/llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll
@@ -1,9 +1,5 @@
-; Run -O2 to make sure that all the usual optimizations do happen before
-; the Hexagon loop idiom recognition runs. This is to check that we still
-; get this opportunity regardless of what happens before.
-
-; RUN: opt -O2 -S < %s | FileCheck %s
-; RUN: opt -passes='default<O2>' -S < %s | FileCheck %s
+; RUN: opt -p 'instcombine,simplifycfg,loop-rotate,loop(hexagon-loop-idiom)' \
+; RUN:   -S %s | FileCheck %s
 
 target triple = "hexagon"
 target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the table lookup better than the pmpy instructions? If not, then I don't think this change is right. You are hiding a legitimate regression.

@kparzysz
Copy link
Contributor

Is the table lookup better than the pmpy instructions? If not, then I don't think this change is right. You are hiding a legitimate regression.

+1

I'd even venture a guess that polynomial multiplication is always better than a table lookup.

@artagnon
Copy link
Contributor Author

Is the table lookup better than the pmpy instructions? If not, then I don't think this change is right. You are hiding a legitimate regression.

+1

I'd even venture a guess that polynomial multiplication is always better than a table lookup.

Hm, in that case, I think we'd have to have the optimization turned off by default, until the necessary work is done to emit pmpy instructions on Hexagon, subsuming HexagonLoopIdiom. We can then turn it on by default, and strip HexagonLoopIdiom, I think.

@artagnon artagnon closed this Jun 18, 2025
@artagnon artagnon deleted the hexagon-pmpy-test branch June 18, 2025 16:42
@kparzysz
Copy link
Contributor

Hm, in that case, I think we'd have to have the optimization turned off by default, until the necessary work is done to emit pmpy instructions on Hexagon, subsuming HexagonLoopIdiom. We can then turn it on by default, and strip HexagonLoopIdiom, I think.

Maybe for now you could add some sort of a target hook to check if the target wants the CRC optimization?

There are other architectures that support polynomial multiplication[1], so adding a target-agnostic support for it would be worthwhile.

[1] https://en.wikipedia.org/wiki/CLMUL_instruction_set

@nikic
Copy link
Contributor

nikic commented Jun 18, 2025

@kparzysz There is some ongoing work for that here: #140301

@artagnon I think it's okay if you just add a triple check for hexagon for now, until it can be integrated into LIR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants