LICM: use IRBuilder in hoist BO assoc

[artagnon]

Use IRBuilder when creating the new invariant instruction, so that the constant-folder has an opportunity to constant-fold the new Instruction that we desire to create.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

Use PatternMatch when matching binary operators in hoistBOAssociation, in order to make it easy to extend (this change is an NFC). Also use IRBuilder when creating the new invariant instruction, so that the constant-folder has an opportunity to constant-fold the new Instruction that we desire to create (this change is not an NFC, and is a minor improvement).

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/LICM.cpp (+20-19)
• (modified) llvm/test/Transforms/LICM/sink-foldable.ll (+1-2)

diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 86c7dceffc5245..6522b6eab1761b 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -2816,40 +2816,41 @@ static bool hoistBOAssociation(Instruction &I, Loop &L,
                                ICFLoopSafetyInfo &SafetyInfo,
                                MemorySSAUpdater &MSSAU, AssumptionCache *AC,
                                DominatorTree *DT) {
-  auto *BO = dyn_cast<BinaryOperator>(&I);
-  if (!BO || !BO->isAssociative())
-    return false;
+  using namespace PatternMatch;
 
-  // Only fold ADDs for now.
+  // Transform "(LV op C1) op C2" ==> "LV op (C1 op C2)"
+  Value *LV, *C1, *C2;
+  if (!match(&I, m_BinOp(m_BinOp(m_Value(LV), m_Value(C1)), m_Value(C2))) ||
+      L.isLoopInvariant(LV) || !L.isLoopInvariant(C1) || !L.isLoopInvariant(C2))
+    return false;
+  auto *BO = cast<BinaryOperator>(&I),
+       *BO0 = cast<BinaryOperator>(BO->getOperand(0));
   Instruction::BinaryOps Opcode = BO->getOpcode();
-  if (Opcode != Instruction::Add)
+  if (BO0->getOpcode() != Opcode || !BO->isAssociative())
     return false;
 
-  auto *BO0 = dyn_cast<BinaryOperator>(BO->getOperand(0));
-  if (!BO0 || BO0->getOpcode() != Opcode || !BO0->isAssociative() ||
-      BO0->hasNUsesOrMore(3))
+  // TODO: Only hoist ADDs for now.
+  if (Opcode != Instruction::Add)
     return false;
 
-  // Transform: "(LV op C1) op C2" ==> "LV op (C1 op C2)"
-  Value *LV = BO0->getOperand(0);
-  Value *C1 = BO0->getOperand(1);
-  Value *C2 = BO->getOperand(1);
-
-  if (L.isLoopInvariant(LV) || !L.isLoopInvariant(C1) || !L.isLoopInvariant(C2))
+  // This is a heuristic to ensure that code-size doesn't blow up.
+  if (BO0->hasNUsesOrMore(3))
     return false;
 
   auto *Preheader = L.getLoopPreheader();
   assert(Preheader && "Loop is not in simplify form?");
 
-  auto *Inv = BinaryOperator::Create(Opcode, C1, C2, "invariant.op",
-                                     Preheader->getTerminator()->getIterator());
+  IRBuilder<> Builder(Preheader->getTerminator());
+  auto *Inv = Builder.CreateBinOp(Opcode, C1, C2, "invariant.op");
+
   auto *NewBO = BinaryOperator::Create(
       Opcode, LV, Inv, BO->getName() + ".reass", BO->getIterator());
 
   // Copy NUW for ADDs if both instructions have it.
-  if (Opcode == Instruction::Add && BO->hasNoUnsignedWrap() &&
-      BO0->hasNoUnsignedWrap()) {
-    Inv->setHasNoUnsignedWrap(true);
+  if (BO->hasNoUnsignedWrap() && BO0->hasNoUnsignedWrap()) {
+    // If the constant-folder didn't kick in, and a new Instruction was created.
+    if (auto *I = dyn_cast<Instruction>(Inv))
+      I->setHasNoUnsignedWrap(true);
     NewBO->setHasNoUnsignedWrap(true);
   }
 
diff --git a/llvm/test/Transforms/LICM/sink-foldable.ll b/llvm/test/Transforms/LICM/sink-foldable.ll
index 36e2eab6313dcd..d1cf3de5301b29 100644
--- a/llvm/test/Transforms/LICM/sink-foldable.ll
+++ b/llvm/test/Transforms/LICM/sink-foldable.ll
@@ -77,7 +77,6 @@ return:
 define ptr @test2(i32 %j, ptr readonly %P, ptr readnone %Q) {
 ; CHECK-LABEL: @test2(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[INVARIANT_OP:%.*]] = add i32 1, 1
 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
 ; CHECK:       for.cond:
 ; CHECK-NEXT:    [[I_ADDR_0:%.*]] = phi i32 [ [[ADD_REASS:%.*]], [[IF_END:%.*]] ]
@@ -98,7 +97,7 @@ define ptr @test2(i32 %j, ptr readonly %P, ptr readnone %Q) {
 ; CHECK-NEXT:    [[ARRAYIDX2:%.*]] = getelementptr inbounds ptr, ptr [[ADD_PTR]], i64 [[IDX2_EXT]]
 ; CHECK-NEXT:    [[L1:%.*]] = load ptr, ptr [[ARRAYIDX2]], align 8
 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp ugt ptr [[L1]], [[Q]]
-; CHECK-NEXT:    [[ADD_REASS]] = add i32 [[I_ADDR]], [[INVARIANT_OP]]
+; CHECK-NEXT:    [[ADD_REASS]] = add i32 [[I_ADDR]], 2
 ; CHECK-NEXT:    br i1 [[CMP2]], label [[LOOPEXIT2:%.*]], label [[FOR_COND]]
 ; CHECK:       loopexit0:
 ; CHECK-NEXT:    [[P0:%.*]] = phi ptr [ null, [[FOR_COND]] ]

[nikic]

The change to use IRBuilder looks good to me, but the rest seems like it will make generalization harder, not easier. You dropped some important checks that are only relevant for generalization, such as the need to check that both binops are associative (this is not implied by having the same opcode) or that the nuw transfer logic is only correct for adds.

[artagnon]

The change to use IRBuilder looks good to me, but the rest seems like it will make generalization harder, not easier.

I hope the PatternMatch change is fine.

You dropped some important checks that are only relevant for generalization, such as the need to check that both binops are associative (this is not implied by having the same opcode) or that the nuw transfer logic is only correct for adds.

Fixed this thinko.

[nikic]

The change to use IRBuilder looks good to me, but the rest seems like it will make generalization harder, not easier.

I hope the PatternMatch change is fine.

It doesn't seem like an improvement, given that you still need to cast<BinaryOperator> for both afterwards.

[artagnon]

The change to use IRBuilder looks good to me, but the rest seems like it will make generalization harder, not easier.

I hope the PatternMatch change is fine.

It doesn't seem like an improvement, given that you still need to cast<BinaryOperator> for both afterwards.

Personally, I find a match to be more readable than multiple getOperand(0) and getOperand(1), but if you feel this is not an improvement, I can drop this hunk.

[nikic]

This PR still does a lot of unnecessary movement of conditions and style changes.

[nikic]

LGTM