[GlobalISel] Fix the infinite loop issue in commute_int_constant_to_rhs

[darkbuck]

• When both operands are constant, the matcher runs into an infinite
  loop as the commutation should be applied only when LHS is a constant
  and RHS is not.

[arsenm]

Maybe add a vector version?

[arsenm]

Can you merge this test with test/CodeGen/AArch64/GlobalISel/combine-const-fold-barrier-rhs.mir

[darkbuck]

Can you merge this test with test/CodeGen/AArch64/GlobalISel/combine-const-fold-barrier-rhs.mir

This test needs --aarch64prelegalizercombiner-disable-rule=constant_fold_binop to trigger issue. That makes the merge complicated. Shall we leave as it is.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: None (darkbuck)

Changes

• When both operands are constant, the matcher runs into an infinite
  loop as the commutation should be applied only when LHS is a constant
  and RHS is not.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+9-8)
• (added) llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir (+28)

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 5cf7a33a5f6756..062132c8304b06 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -6276,14 +6276,15 @@ bool CombinerHelper::matchShiftsTooBig(MachineInstr &MI) {
 bool CombinerHelper::matchCommuteConstantToRHS(MachineInstr &MI) {
   Register LHS = MI.getOperand(1).getReg();
   Register RHS = MI.getOperand(2).getReg();
-  auto *LHSDef = MRI.getVRegDef(LHS);
-  if (getIConstantVRegVal(LHS, MRI).has_value())
-    return true;
-
-  // LHS may be a G_CONSTANT_FOLD_BARRIER. If so we commute
-  // as long as we don't already have a constant on the RHS.
-  if (LHSDef->getOpcode() != TargetOpcode::G_CONSTANT_FOLD_BARRIER)
-    return false;
+  if (!getIConstantVRegVal(LHS, MRI)) {
+    // Skip commuting if LHS is not a constant. But, LHS may be a
+    // G_CONSTANT_FOLD_BARRIER. If so we commute as long as we don't already
+    // have a constant on the RHS.
+    if (MRI.getVRegDef(LHS)->getOpcode() !=
+        TargetOpcode::G_CONSTANT_FOLD_BARRIER)
+      return false;
+  }
+  // Commute as long as RHS is not a constant or G_CONSTANT_FOLD_BARRIER.
   return MRI.getVRegDef(RHS)->getOpcode() !=
              TargetOpcode::G_CONSTANT_FOLD_BARRIER &&
          !getIConstantVRegVal(RHS, MRI);
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir b/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir
new file mode 100644
index 00000000000000..b145a6d3fd39d1
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir
@@ -0,0 +1,28 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
+# RUN: llc -mtriple aarch64 -run-pass=aarch64-prelegalizer-combiner %s -o - \
+# RUN:     --aarch64prelegalizercombiner-disable-rule=constant_fold_binop | FileCheck %s
+
+# `constant_fold_binop` is disabled to trigger the infinite loop in `commute_int_constant_to_rhs`.
+
+---
+name:            add
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $s0
+
+    ; CHECK-LABEL: name: add
+    ; CHECK: liveins: $s0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %c0:_(s32) = G_CONSTANT i32 1
+    ; CHECK-NEXT: %c1:_(s32) = G_CONSTANT i32 2
+    ; CHECK-NEXT: %add:_(s32) = G_ADD %c0, %c1
+    ; CHECK-NEXT: $s0 = COPY %add(s32)
+    ; CHECK-NEXT: RET_ReallyLR
+    %c0:_(s32) = G_CONSTANT i32 1
+    %c1:_(s32) = G_CONSTANT i32 2
+    %add:_(s32) = G_ADD %c0, %c1
+    $s0 = COPY %add
+    RET_ReallyLR
+
+...

[RKSimon]

@darkbuck This is failing on some buildbots - please can you take a look? https://lab.llvm.org/buildbot/#/builders/124/builds/10375

[danilaml]

I also saw similar failures when compiling in release mode (interestingly, not for debug builds).

[annamthomas]

@darkbuck We are seeing the same failing tests on our downstream builds because of this test failure: https://lab.llvm.org/buildbot/#/builders/124/builds/10375
Given one of the buildbots failed (and I'm not sure what the fix is here), can we please revert to green? https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

[gulfemsavrun]

This test also failed in our Clang toolchain builders.

FAIL: LLVM :: CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir (2372 of 53803)
******************** TEST 'LLVM :: CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir' FAILED ********************
Exit Code: 2

Command Output (stderr):
--
RUN: at line 2: /b/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/bin/llc -mtriple aarch64 -run-pass=aarch64-prelegalizer-combiner /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir -o -      --aarch64prelegalizercombiner-disable-rule=constant_fold_binop | /b/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir
+ /b/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir
+ /b/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/bin/llc -mtriple aarch64 -run-pass=aarch64-prelegalizer-combiner /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir -o - --aarch64prelegalizercombiner-disable-rule=constant_fold_binop
LLVM ERROR: Invalid rule identifier
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/combine-commute-int-const-lhs.mir

--

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8751607530180046481/+/u/clang/test/stdout

[annamthomas]

Okay, I'll try reverting the change given these buildbot failures. @darkbuck pls feel free to reland with fix(es).

Ah, I see @gulfemsavrun already reverted. Thanks!