[InstCombine] Fold icmp(constants[x]) when the range of x is given #67093

XChy · 2023-09-22T07:00:03Z

This patch extends foldCmpLoadFromIndexedGlobal and switch to byte-driven method to fold IR below:

define i1 @cmp_load_constant_array0(i64 %x){
entry:
  %cond = icmp ult i64 %x, 2
  br i1 %cond, label %case1, label %case2

case2:
  ret i1 0

case1:
  %isOK_ptr = getelementptr inbounds i32, ptr @CG, i64 %x
  %isOK = load i32, ptr %isOK_ptr
  %cond_inferred = icmp ult i32 %isOK, 3
  ret i1 %cond_inferred
}

Proof:
alive2

Related issue:
#64238

Migrated from Phabricator

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

PR Link: llvm/llvm-project#67093

github-actions · 2023-12-22T17:41:18Z

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

XChy · 2023-12-23T18:22:25Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+  if (BeginOffset.slt(0))
+    BeginOffset += OffsetStep;
+
+  uint64_t ElementCountToTraverse = (DataSize - BeginOffset).udiv(OffsetStep).getZExtValue() + 1;


I missed "+1" here, actually BeginOffset indeed includes one more element. Fix it now.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

dtcxzyw · 2023-12-29T09:17:45Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+    // If the index is larger than the pointer offset size of the target,
+    // truncate the index down like the GEP would do implicitly.  We don't have
+    // to do this for an inbounds GEP because the index can't be out of range.
+    if (!GEP->isInBounds() && IdxBitWidth > IndexSize)


As we canonicalize the index's type of GEPs, I think we can skip the transform when IdxBitWidth != IndexSize.

dtcxzyw · 2023-12-29T09:29:04Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+    Value *Idx = ConstantInt::get(
+        PtrIdxTy, (ConstantOffset - BeginOffset).sdiv(OffsetStep));
+    uint64_t IdxBitWidth = Idx->getType()->getScalarSizeInBits();
+    for (auto [Var, Coefficient] : VariableOffsets) {


The size of VariableOffset is 1.

dtcxzyw · 2023-12-29T09:32:27Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp


    if (Ty) {
-      Idx = MaskIdx(Idx);
+      Idx = LazyGetIndex(Idx);
      Value *V = Builder.CreateIntCast(Idx, Ty, false);
      V = Builder.CreateLShr(ConstantInt::get(Ty, MagicBitvector), V);
      V = Builder.CreateAnd(ConstantInt::get(Ty, 1), V);


Please avoid creating multiple instructions when the load has multiple users.
See also https://github.com/dtcxzyw/llvm-opt-benchmark/pull/28/files/f845e103a2e2a78409e1f2aed9e21733056fd134#r1435245448.
But it would be good to do it in a separate PR.

OK, I will post a separate PR for it later.

nikic · 2024-01-02T14:32:18Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+    auto [Var, Coefficient] = VariableOffsets.front();
+    uint64_t VarBitWidth = Var->getType()->getScalarSizeInBits();
+    assert("GEP indices do not get canonicalized to the index type" &&
+           VarBitWidth == IdxBitWidth);


You should check this condition at the start of the transform and bail out. While it is canonicalized, there's no guarantee that the GEP is canonicalized at this point yet.

nikic · 2024-01-02T14:32:37Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+    // idx < 3, we actually get x + 3 < 3
+    Value *Bias = ConstantInt::get(
+        PtrIdxTy, (ConstantOffset - BeginOffset).sdiv(OffsetStep));
+    uint64_t IdxBitWidth = PtrIdxTy->getScalarSizeInBits();


This is the same as the IndexSize variable.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

llvm/test/Transforms/InstCombine/load-cmp.ll

nikic · 2024-01-02T14:36:17Z

llvm/test/Transforms/InstCombine/load-cmp.ll

+;
+entry:
+  %cond = icmp ult i64 %x, 2
+  br i1 %cond, label %case1, label %case2


I think it would be better to omit this condition from the test, so we can see the direct result of the transform, without additional implication reasoning. Same for the next test.

nikic · 2024-01-02T14:38:40Z

llvm/test/Transforms/InstCombine/load-cmp.ll

+  %isOK = load i32, ptr %isOK_ptr
+  %cond_inferred = icmp ult i32 %isOK, %y
+  ret i1 %cond_inferred
+}


I'd like to see some tests where we also apply an additional offset. In particular also if the offset is greater than the stride, and if the offset is negative. (Preferably for a "non-messy" case, to make it understandable.)

XChy requested review from nikic and goldsteinn September 22, 2023 07:00

llvmbot added the llvm:transforms label Sep 22, 2023

XChy force-pushed the IndexedGlobal branch from 142892d to 2a01a60 Compare September 22, 2023 07:56

XChy linked an issue Sep 26, 2023 that may be closed by this pull request

[InstCombine] Missed optimization for icmp(constants[x]) when the range of x is implied #64238

Open

nikic mentioned this pull request Dec 21, 2023

[InstCombine] Canonicalize constant GEPs to i8 source element type #68882

Merged

XChy force-pushed the IndexedGlobal branch from 2a01a60 to 6dcfc41 Compare December 22, 2023 05:17

nikic reviewed Dec 22, 2023

View reviewed changes

nikic requested a review from dtcxzyw December 22, 2023 12:02

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Dec 22, 2023

pre-commit: test PR67093

a8e9dab

PR Link: llvm/llvm-project#67093

dtcxzyw mentioned this pull request Dec 22, 2023

pre-commit: test PR67093 dtcxzyw/llvm-opt-benchmark#28

Open

XChy force-pushed the IndexedGlobal branch from 6dcfc41 to 54d4920 Compare December 22, 2023 17:39

XChy force-pushed the IndexedGlobal branch from 54d4920 to 6a0b79e Compare December 23, 2023 18:12

XChy commented Dec 23, 2023

View reviewed changes

XChy force-pushed the IndexedGlobal branch 2 times, most recently from d78ded4 to 353c5f3 Compare December 23, 2023 18:48

dtcxzyw requested changes Dec 29, 2023

View reviewed changes

dtcxzyw reviewed Dec 29, 2023

View reviewed changes

XChy force-pushed the IndexedGlobal branch 3 times, most recently from 956ee50 to 992f410 Compare December 29, 2023 13:04

nikic reviewed Jan 2, 2024

View reviewed changes

XChy added 2 commits January 16, 2024 21:16

[InstCombine] Tests for simplifying icmp(constants[x])

7e0b124

[InstCombine] Fold icmp(constants[x]) when the range of x is given

5dd5113

XChy force-pushed the IndexedGlobal branch from 992f410 to 5dd5113 Compare January 16, 2024 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Fold icmp(constants[x]) when the range of x is given #67093

[InstCombine] Fold icmp(constants[x]) when the range of x is given #67093

XChy commented Sep 22, 2023

github-actions bot commented Dec 22, 2023 •

edited

XChy Dec 23, 2023

dtcxzyw Dec 29, 2023

dtcxzyw Dec 29, 2023

dtcxzyw Dec 29, 2023

XChy Dec 29, 2023

nikic Jan 2, 2024

nikic Jan 2, 2024

nikic Jan 2, 2024

nikic Jan 2, 2024

[InstCombine] Fold icmp(constants[x]) when the range of x is given #67093

Are you sure you want to change the base?

[InstCombine] Fold icmp(constants[x]) when the range of x is given #67093

Conversation

XChy commented Sep 22, 2023

github-actions bot commented Dec 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 22, 2023 •

edited