[InstCombine] Prevent infinite loop with two shifts

[momo5502]

The following pattern: (C2 << X) << C1 will usually be transformed into (C2 << C1) << X, essentially swapping X and C1.

However, this should only be done when C1 is an immediate constant, otherwise this
can lead to both constants being swapped forever

This fixes #118798

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Maurice Heumann (momo5502)

Changes

The following pattern: (C2 << X) << C1 will usually be transformed into (C2 << C1) << X, essentially swapping X and C1.

However, this should not be done when X is also a constant, as this can lead to both constants being swapped forever.

This fixes #118798

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp (+3-1)
• (added) llvm/test/Transforms/InstCombine/shl-twice-constant.ll (+22)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index 10c3ccdb2243a1..3fb094579d585a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -786,7 +786,9 @@ Instruction *InstCombinerImpl::FoldShiftByConstant(Value *Op0, Constant *C1,
   Constant *C2;
   Value *X;
   bool IsLeftShift = I.getOpcode() == Instruction::Shl;
-  if (match(Op0, m_BinOp(I.getOpcode(), m_ImmConstant(C2), m_Value(X)))) {
+  if (match(Op0,
+            m_BinOp(I.getOpcode(), m_ImmConstant(C2),
+                    m_CombineAnd(m_Value(X), m_Unless(m_Constant()))))) {
     Instruction *R = BinaryOperator::Create(
         I.getOpcode(), Builder.CreateBinOp(I.getOpcode(), C2, C1), X);
     BinaryOperator *BO0 = cast<BinaryOperator>(Op0);
diff --git a/llvm/test/Transforms/InstCombine/shl-twice-constant.ll b/llvm/test/Transforms/InstCombine/shl-twice-constant.ll
new file mode 100644
index 00000000000000..54c5611400f46c
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/shl-twice-constant.ll
@@ -0,0 +1,22 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+@c = external constant i8
+@c2 = external constant i8
+
+define i16 @testfunc() {
+; CHECK-LABEL: @testfunc(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = shl nuw i64 1, ptrtoint (ptr @c2 to i64)
+; CHECK-NEXT:    [[TMP1:%.*]] = shl i64 [[TMP0]], ptrtoint (ptr @c to i64)
+; CHECK-NEXT:    [[TMP2:%.*]] = inttoptr i64 [[TMP1]] to ptr
+; CHECK-NEXT:    [[TMP3:%.*]] = load i16, ptr [[TMP2]], align 1
+; CHECK-NEXT:    ret i16 [[TMP3]]
+;
+entry:
+  %0 = shl i64 1, ptrtoint (ptr @c2 to i64)
+  %1 = shl i64 %0, ptrtoint (ptr @c to i64)
+  %2 = inttoptr i64 %1 to ptr
+  %3 = load i16, ptr %2, align 1
+  ret i16 %3
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

I think it may be better to address this by replacing the check in

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp

Line 430 in 5977d79

if (Constant *CUI = dyn_cast<Constant>(Op1))

with a check for ImmConstant.

[momo5502]

I think it may be better to address this by replacing the check in

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp

Line 430 in 5977d79

if (Constant *CUI = dyn_cast<Constant>(Op1))

with a check for ImmConstant.

done

[nikic]

LG

[nikic]

The last two instructions in this test shouldn't be needed, it can be reduced to:

@c = external constant i8
@c2 = external constant i8

define i64 @testfunc() {
  %shl1 = shl i64 1, ptrtoint (ptr @c2 to i64)
  %shl2 = shl i64 %shl1, ptrtoint (ptr @c to i64)
  ret i64 %shl2
}
```.

[momo5502]

test adjusted. thank you for the review :)

[lukel97]

I think this might have broken some combines with scalable vectors, beforehand this

define <vscale x 4 x i8> @f(<vscale x 4 x i8> %x) {
  %1 = and <vscale x 4 x i8> %x, splat (i8 2)
  %2 = shl <vscale x 4 x i8> %1, splat (i8 2)
  ret <vscale x 4 x i8> %2
}

used to get combined to

define <vscale x 4 x i8> @f(<vscale x 4 x i8> %x) {
  %1 = shl <vscale x 4 x i8> %x, splat (i8 2)
  %2 = and <vscale x 4 x i8> %1, splat (i8 8)
  ret <vscale x 4 x i8> %2
}

But now this fails because a scalable splat vector is a constant expression IIUC. Maybe we need some sort of pattern matcher that matches constant immediates + constant splats?