Upgrade TypePromotionTransaction to be able to report changes in CodeGenPrepare

[serge-sans-paille]

Summary

optimizeMemoryInst was reporting no change while still modifying the IR.
Inspect the status of TypePromotionTransaction to get a better status.

Related to https://reviews.llvm.org/D80916

[foad]

"advocating" should be "reporting" (or perhaps "advertising"?).

[serge-sans-paille]

up?

[serge-sans-paille]

@foad / @nikic does that look good to you?

[foad]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2952

You don't need the outer parens.

4963

The commit message suggests you're just fixing the return value from optimizeMemoryInst, but this change (not rolling back) looks like a change in behaviour. Can you explain?

[serge-sans-paille]

Address reviewer comment.

[serge-sans-paille]

Apply fix suggested in review

[serge-sans-paille]

llvm/lib/CodeGen/CodeGenPrepare.cpp

4963

Thanks for spotting this - it was an error.

[serge-sans-paille]

@foad should look better now, right?

[serge-sans-paille]

Up!

[serge-sans-paille]

@foad / @jdoerfert up ? This is the last missing piece before I can land the pass return status expensive check, aka https://reviews.llvm.org/D80916

[foad]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2950

Maybe changedSince would be a better name?

4962

I still don't really understand whether this is an intentional change in behaviour, or just fixing the return value from optimizeMemoryInst.

[serge-sans-paille]

Stick to original API for easier maintenance.

llvm/lib/CodeGen/CodeGenPrepare.cpp

2950

I've reworked the patch to make it closer to the original code, making that change no longer necessary.

[foad]

Looks good to me now - though it would still be nice to hear from @qcolombet as the author of TypePromotionTransaction.

[serge-sans-paille]

In D81256#2135457, @foad wrote:

Looks good to me now - though it would still be nice to hear from @qcolombet as the author of TypePromotionTransaction.

Thanks! I'll leave another day before I merge it if we don't hear from @qcolombet

llvm/lib/CodeGen/CodeGenPrepare.cpp

public:
   /// Restoration point.
   /// The restoration point is a pointer to an action instead of an iterator
   /// because the iterator may be invalidated but not the pointer.
   using ConstRestorationPt = const TypePromotionAction *;
 
   TypePromotionTransaction(SetOfInstrs &RemovedInsts)
       : RemovedInsts(RemovedInsts) {}
 
-  /// Advocate every changes made in that transaction.
-  void commit();
+  /// Advocate every changes made in that transaction. Return true if any change
+  /// happen.
+  bool commit();
 
   /// Undo all the changes made after the given point.
   void rollback(ConstRestorationPt Point);
 
   /// Get the current restoration point.
   ConstRestorationPt getRestorationPoint() const;
 
   /// \name API for IR modification with state keeping to support rollback.
Actions.push_back(
           Inst, Before));
 }
 
 TypePromotionTransaction::ConstRestorationPt
 TypePromotionTransaction::getRestorationPoint() const {
   return !Actions.empty() ? Actions.back().get() : nullptr;
 }
 
-void TypePromotionTransaction::commit() {
+bool TypePromotionTransaction::commit() {
   for (CommitPt It = Actions.begin(), EndIt = Actions.end(); It != EndIt;
        ++It)
     (*It)->commit();
+  bool Modified = !Actions.empty();
   Actions.clear();
+  return Modified;
 }
 
 void TypePromotionTransaction::rollback(
     TypePromotionTransaction::ConstRestorationPt Point) {
   while (!Actions.empty() && Point != Actions.back().get()) {
     std::unique_ptr<TypePromotionAction> Curr = Actions.pop_back_val();
     Curr->undo();
   }
 }
 
 namespace {
 
 /// A helper class for matching addressing modes.
 ///
 /// This encapsulates the logic for matching the target-legal addressing modes.
 class AddressingModeMatcher {
   SmallVectorImpl<Instruction*> &AddrModeInsts;
   const TargetLowering &TLI;
   const TargetRegisterInfo &TRI;
   const DataLayout &DL;
 
if (!AddrModes.addNewAddrMode(NewAddrMode))
       break;
   }
 
   // Try to combine the AddrModes we've collected. If we couldn't collect any,
   // or we have multiple but either couldn't combine them or combining them
   // wouldn't do anything useful, bail out now.
   if (!AddrModes.combineAddrModes()) {
     TPT.rollback(LastKnownGood);
     return false;
   }
-  TPT.commit();
+  bool Modified = TPT.commit();
 
   // Get the combined AddrMode (or the only AddrMode, if we only had one).
   ExtAddrMode AddrMode = AddrModes.getAddrMode();
 
   // If all the instructions matched are already in this BB, don't do anything.
   // If we saw a Phi node then it is not local definitely, and if we saw a select
   // then we want to push the address calculation past it even if it's already
   // in this BB.
   if (!PhiOrSelectSeen && none_of(AddrModeInsts, [&](Value *V) {
         return IsNonLocalValue(V, MemoryInst->getParent());
                   })) {
     LLVM_DEBUG(dbgs() << "CGP: Found      local addrmode: " << AddrMode
                       << "\n");
-    return false;
+    return Modified;
   }
 
   // Insert this computation right after this user.  Since our caller is
   // scanning from the top of the BB to the bottom, reuse of the expr are
   // guaranteed to happen later.
   IRBuilder<> Builder(MemoryInst);
 
   // Now that we determined the addressing expression we want to use and know
if (AddrMode.BaseReg && AddrMode.BaseReg->getType()->isPointerTy()) {
       ResultPtr = AddrMode.BaseReg;
       AddrMode.BaseReg = nullptr;
     }
 
     if (AddrMode.Scale && AddrMode.ScaledReg->getType()->isPointerTy()) {
       // We can't add more than one pointer together, nor can we scale a
       // pointer (both of which seem meaningless).
       if (ResultPtr || AddrMode.Scale != 1)
-        return false;
+        return Modified;
 
       ResultPtr = AddrMode.ScaledReg;
       AddrMode.Scale = 0;
     }
 
     // It is only safe to sign extend the BaseReg if we know that the math
     // required to create it did not overflow before we extend it. Since
     // the original IR value was tossed in favor of a constant back when
     // the AddrMode was created we need to bail out gracefully if widths
     // do not match instead of extending it.
     //
     // (See below for code to add the scale.)
     if (AddrMode.Scale) {
       Type *ScaledRegTy = AddrMode.ScaledReg->getType();
       if (cast<IntegerType>(IntPtrTy)->getBitWidth() >
           cast<IntegerType>(ScaledRegTy)->getBitWidth())
-        return false;
+        return Modified;
     }
 
     if (AddrMode.BaseGV) {
       if (ResultPtr)
-        return false;
+        return Modified;
 
       ResultPtr = AddrMode.BaseGV;
     }
 
     // If the real base value actually came from an inttoptr, then the matcher
     // will look through it and provide only the integer value. In that case,
     // use it here.
     if (!DL->isNonIntegralPointerType(Addr->getType())) {
       if (!ResultPtr && AddrMode.BaseReg) {
         ResultPtr = Builder.CreateIntToPtr(AddrMode.BaseReg, Addr->getType(),
                                            "sunkaddr");
         AddrMode.BaseReg = nullptr;
       } else if (!ResultPtr && AddrMode.Scale == 1) {
         ResultPtr = Builder.CreateIntToPtr(AddrMode.ScaledReg, Addr->getType(),
                                            "sunkaddr");
         AddrMode.Scale = 0;
       }
     }
 
     if (!ResultPtr &&
         !AddrMode.BaseReg && !AddrMode.Scale && !AddrMode.BaseOffs) {
       SunkAddr = Constant::getNullValue(Addr->getType());
     } else if (!ResultPtr) {
-      return false;
+      return Modified;
     } else {
       Type *I8PtrTy =
           Builder.getInt8PtrTy(Addr->getType()->getPointerAddressSpace());
       Type *I8Ty = Builder.getInt8Ty();
 
       // Start with the base register. Do this first so that subsequent address
       // matching finds it last, which will prevent it from trying to match it
       // as the scaled value in case it happens to be a mul. That would be
if (SunkAddr) {
     Type *ScaleTy = AddrMode.Scale ? AddrMode.ScaledReg->getType() : nullptr;
     PointerType *BasePtrTy = dyn_cast_or_null<PointerType>(BaseTy);
     PointerType *ScalePtrTy = dyn_cast_or_null<PointerType>(ScaleTy);
     if (DL->isNonIntegralPointerType(Addr->getType()) ||
         (BasePtrTy && DL->isNonIntegralPointerType(BasePtrTy)) ||
         (ScalePtrTy && DL->isNonIntegralPointerType(ScalePtrTy)) ||
         (AddrMode.BaseGV &&
          DL->isNonIntegralPointerType(AddrMode.BaseGV->getType())))
-      return false;
+      return Modified;
 
     LLVM_DEBUG(dbgs() << "CGP: SINKING nonlocal addrmode: " << AddrMode
                       << " for " << *MemoryInst << "\n");
     Type *IntPtrTy = DL->getIntPtrType(Addr->getType());
     Value *Result = nullptr;
 
     // Start with the base register. Do this first so that subsequent address
     // matching finds it last, which will prevent it from trying to match it
if (AddrMode.Scale) {
         // It is only safe to sign extend the BaseReg if we know that the math
         // required to create it did not overflow before we extend it. Since
         // the original IR value was tossed in favor of a constant back when
         // the AddrMode was created we need to bail out gracefully if widths
         // do not match instead of extending it.
         Instruction *I = dyn_cast_or_null<Instruction>(Result);
         if (I && (Result != AddrMode.BaseReg))
           I->eraseFromParent();
-        return false;
+        return Modified;
       }
       if (AddrMode.Scale != 1)
         V = Builder.CreateMul(V, ConstantInt::get(IntPtrTy, AddrMode.Scale),
                               "sunkaddr");
       if (Result)
         Result = Builder.CreateAdd(Result, V, "sunkaddr");
       else
         Result = V;