-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[ARM] Restore hasSideEffects flag on t2WhileLoopSetup #168948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-backend-arm Author: Sergei Barannikov (s-barannikov) ChangesARM relies on deprecated TableGen behavior of guessing instruction properties from patterns ( Before #168209, TableGen conservatively guessed that t2WhileLoopSetup has side effects because the instruction wasn't matched by any pattern. After the patch, TableGen guesses it has no side effects because the added pattern uses only Add SDNPSideEffect to the node so that TableGen guesses the property right, and also Full diff: https://github.com/llvm/llvm-project/pull/168948.diff 2 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMInstrThumb2.td b/llvm/lib/Target/ARM/ARMInstrThumb2.td
index 317959c0342f7..19c179be5b987 100644
--- a/llvm/lib/Target/ARM/ARMInstrThumb2.td
+++ b/llvm/lib/Target/ARM/ARMInstrThumb2.td
@@ -5584,7 +5584,8 @@ class t2LOL<dag oops, dag iops, string asm, string ops>
// Setup for the iteration count of a WLS. See t2WhileLoopSetup.
def arm_wlssetup
: SDNode<"ARMISD::WLSSETUP",
- SDTypeProfile<1, 1, [SDTCisInt<0>, SDTCisSameAs<1, 0>]>>;
+ SDTypeProfile<1, 1, [SDTCisInt<0>, SDTCisSameAs<1, 0>]>,
+ [SDNPSideEffect]>;
// Low-overhead loops, While Loop Start branch. See t2WhileLoopStart
def arm_wls : SDNode<"ARMISD::WLS",
diff --git a/llvm/test/CodeGen/Thumb2/LowOverheadLoops/pr168209.ll b/llvm/test/CodeGen/Thumb2/LowOverheadLoops/pr168209.ll
new file mode 100644
index 0000000000000..a6dded12c064b
--- /dev/null
+++ b/llvm/test/CodeGen/Thumb2/LowOverheadLoops/pr168209.ll
@@ -0,0 +1,45 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 6
+; RUN: llc -mtriple=thumbv8.1m.main %s -o - | FileCheck %s
+
+; Checks that t2WhileLoopSetup is not CSEd.
+
+define i32 @test(i16 %arg) {
+; CHECK-LABEL: test:
+; CHECK: @ %bb.0: @ %bb
+; CHECK-NEXT: push {r7, lr}
+; CHECK-NEXT: uxth r0, r0
+; CHECK-NEXT: wls lr, r0, .LBB0_4
+; CHECK-NEXT: .LBB0_1: @ %bb3
+; CHECK-NEXT: @ =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: le lr, .LBB0_1
+; CHECK-NEXT: @ %bb.2: @ %bb2
+; CHECK-NEXT: wls lr, r0, .LBB0_4
+; CHECK-NEXT: .LBB0_3: @ %bb7
+; CHECK-NEXT: @ =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: le lr, .LBB0_3
+; CHECK-NEXT: .LBB0_4: @ %.critedge
+; CHECK-NEXT: movs r0, #0
+; CHECK-NEXT: pop {r7, pc}
+bb:
+ %i = zext i16 %arg to i32
+ %i1 = icmp eq i16 %arg, 0
+ br i1 %i1, label %.critedge, label %bb3
+
+bb2: ; preds = %bb3
+ br i1 %i1, label %.critedge, label %bb7
+
+bb3: ; preds = %bb3, %bb
+ %i4 = phi i32 [ %i5, %bb3 ], [ 0, %bb ]
+ %i5 = add i32 %i4, 1
+ %i6 = icmp eq i32 %i5, %i
+ br i1 %i6, label %bb2, label %bb3
+
+bb7: ; preds = %bb7, %bb2
+ %i8 = phi i32 [ %i9, %bb7 ], [ 0, %bb2 ]
+ %i9 = add i32 %i8, 1
+ %i10 = icmp eq i32 %i9, %i
+ br i1 %i10, label %.critedge, label %bb7
+
+.critedge: ; preds = %bb7, %bb2, %bb
+ ret i32 0
+}
|
ARM relies on deprecated TableGen behavior of guessing instruction properties from patterns (`def ARM : Target` doesn't have `guessInstructionProperties` set to false). Before llvm#168209, TableGen conservatively guessed that t2WhileLoopSetup has side effects because the instruction wasn't matched by any pattern. After the patch, TableGen guesses it has no side effects because the added pattern uses only `arm_wlssetup` node, which has no side effects. Add SDNPSideEffect to the node so that TableGen guesses the property right, and also `hasSideEffects = 1` to the instruction in case ARM ever sets `guessInstructionProperties` to false.
04b5870 to
a71b696
Compare
🐧 Linux x64 Test Results
|
VladiKrapp-Arm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked that this does resolve the crash.
LGTM, but I'd appreciate a review from someone a bit more familiar with the domain.
davemgreen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. LGTM
ARM relies on deprecated TableGen behavior of guessing instruction properties from patterns (
def ARM : Targetdoesn't haveguessInstructionPropertiesset to false).Before #168209, TableGen conservatively guessed that
t2WhileLoopSetuphas side effects because the instruction wasn't matched by any pattern.After the patch, TableGen guesses it has no side effects because the added pattern uses only
arm_wlssetupnode, which has no side effects.Add
SDNPSideEffectto the node so that TableGen guesses the property right, and alsohasSideEffects = 1to the instruction in case ARM ever setsguessInstructionPropertiesto false.