InstrProfiling.cpp: Let lowerIntrinsics() able to insert BBs.

[chapuni]

IsBBChanged should be set when lowering inserted BBs. Make lowerMCDCTestVectorBitmapUpdate() return IsBBChanged unconditionally at the moment.

[llvmbot]

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: NAKAMURA Takumi (chapuni)

Changes

IsBBChanged should be set when lowering inserted BBs. Make lowerMCDCTestVectorBitmapUpdate() return IsBBChanged unconditionally at the moment.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp (+40-29)

diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index 0c79eaa812b5f..e9bccae1546c1 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -276,7 +276,7 @@ class InstrLowerer final {
 
   /// Replace instrprof.mcdc.tvbitmask.update with a shift and or instruction
   /// using the index represented by the a temp value into a bitmap.
-  void lowerMCDCTestVectorBitmapUpdate(InstrProfMCDCTVBitmapUpdate *Ins);
+  bool lowerMCDCTestVectorBitmapUpdate(InstrProfMCDCTVBitmapUpdate *Ins);
 
   /// Replace instrprof.mcdc.temp.update with a shift and or instruction using
   /// the corresponding condition ID.
@@ -625,35 +625,45 @@ PreservedAnalyses InstrProfilingLoweringPass::run(Module &M,
 bool InstrLowerer::lowerIntrinsics(Function *F) {
   bool MadeChange = false;
   PromotionCandidates.clear();
-  for (BasicBlock &BB : *F) {
-    for (Instruction &Instr : llvm::make_early_inc_range(BB)) {
-      if (auto *IPIS = dyn_cast<InstrProfIncrementInstStep>(&Instr)) {
-        lowerIncrement(IPIS);
-        MadeChange = true;
-      } else if (auto *IPI = dyn_cast<InstrProfIncrementInst>(&Instr)) {
-        lowerIncrement(IPI);
-        MadeChange = true;
-      } else if (auto *IPC = dyn_cast<InstrProfTimestampInst>(&Instr)) {
-        lowerTimestamp(IPC);
-        MadeChange = true;
-      } else if (auto *IPC = dyn_cast<InstrProfCoverInst>(&Instr)) {
-        lowerCover(IPC);
-        MadeChange = true;
-      } else if (auto *IPVP = dyn_cast<InstrProfValueProfileInst>(&Instr)) {
-        lowerValueProfileInst(IPVP);
-        MadeChange = true;
-      } else if (auto *IPMP = dyn_cast<InstrProfMCDCBitmapParameters>(&Instr)) {
-        IPMP->eraseFromParent();
-        MadeChange = true;
-      } else if (auto *IPBU = dyn_cast<InstrProfMCDCTVBitmapUpdate>(&Instr)) {
-        lowerMCDCTestVectorBitmapUpdate(IPBU);
-        MadeChange = true;
-      } else if (auto *IPTU = dyn_cast<InstrProfMCDCCondBitmapUpdate>(&Instr)) {
-        lowerMCDCCondBitmapUpdate(IPTU);
-        MadeChange = true;
+  bool IsBBChanged;
+  do {
+    IsBBChanged = false;
+    for (BasicBlock &BB : *F) {
+      for (Instruction &Instr : llvm::make_early_inc_range(BB)) {
+        if (auto *IPIS = dyn_cast<InstrProfIncrementInstStep>(&Instr)) {
+          lowerIncrement(IPIS);
+          MadeChange = true;
+        } else if (auto *IPI = dyn_cast<InstrProfIncrementInst>(&Instr)) {
+          lowerIncrement(IPI);
+          MadeChange = true;
+        } else if (auto *IPC = dyn_cast<InstrProfTimestampInst>(&Instr)) {
+          lowerTimestamp(IPC);
+          MadeChange = true;
+        } else if (auto *IPC = dyn_cast<InstrProfCoverInst>(&Instr)) {
+          lowerCover(IPC);
+          MadeChange = true;
+        } else if (auto *IPVP = dyn_cast<InstrProfValueProfileInst>(&Instr)) {
+          lowerValueProfileInst(IPVP);
+          MadeChange = true;
+        } else if (auto *IPMP =
+                       dyn_cast<InstrProfMCDCBitmapParameters>(&Instr)) {
+          IPMP->eraseFromParent();
+          MadeChange = true;
+        } else if (auto *IPBU = dyn_cast<InstrProfMCDCTVBitmapUpdate>(&Instr)) {
+          IsBBChanged = lowerMCDCTestVectorBitmapUpdate(IPBU);
+          MadeChange = true;
+        } else if (auto *IPTU =
+                       dyn_cast<InstrProfMCDCCondBitmapUpdate>(&Instr)) {
+          lowerMCDCCondBitmapUpdate(IPTU);
+          MadeChange = true;
+        }
+        if (IsBBChanged)
+          break;
       }
+      if (IsBBChanged)
+        break;
     }
-  }
+  } while (IsBBChanged);
 
   if (!MadeChange)
     return false;
@@ -1007,7 +1017,7 @@ void InstrLowerer::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
   CoverageNamesVar->eraseFromParent();
 }
 
-void InstrLowerer::lowerMCDCTestVectorBitmapUpdate(
+bool InstrLowerer::lowerMCDCTestVectorBitmapUpdate(
     InstrProfMCDCTVBitmapUpdate *Update) {
   IRBuilder<> Builder(Update);
   auto *Int8Ty = Type::getInt8Ty(M.getContext());
@@ -1051,6 +1061,7 @@ void InstrLowerer::lowerMCDCTestVectorBitmapUpdate(
   //  store i8 %8, ptr %3, align 1
   Builder.CreateStore(Result, BitmapByteAddr);
   Update->eraseFromParent();
+  return false;
 }
 
 void InstrLowerer::lowerMCDCCondBitmapUpdate(

[ornata]

What is the difference between MadeChange and IsBBChanged?

[chapuni]

@ornata MadeChange is a just trigger to the posprocess promoteCounterLoadStores(F).
All intrinsics was replaced within BB Insts in the current impl.

I would like to introduce BB change to insert conditional store. This makes for (BB) harder and redoing the loop is required.
This is the reason why I introduce IsBBChanged.

Do you have any better idea for redoing if BB is added by SplitBlockAndInsertIfThen()?
Rather redoing the loop, could we record IntrinsicInsts at 1st loop and procees marked Insts in 2nd loop? For example,

  SmallVector<IntrinsicInst*> InstrsLowered;
  for (BB: *F)
    for (Instr: BB)
      if (isa<IntrinsicInst>(Instr) && (isCounterBase(Instr) || isMCDCBase(Instr))
        InstrsLowered.push_back(Instr);

  // This can replace MadeChange.
  if (InstrsLowered.empty()) return false;

  for (Instr : InstrLowered)
    if (auto *IPFOO == dyn_cast<InstrProfFOO>(Instr)
      lowerFOO(IPFOO);
    else if ...

promoteCounterLoadStores(F);
return true;  

This will avoid flags and redoing.

[ellishg]

IRPGO profiles will only match if the CFG of the instrumented function is the same as the CFG of optimized function. For this reason, the instrumentation should not change the CFG of the function.

If you want a conditional store, you might want to consider creating a new function and calling it. You might even be able to force it to be inlined. If the CFG of the assembly changes, that is ok. We just want the CFG of the IR to stay the same.

[chapuni]

@ellishg Thanks for explanation of backgrounds. I've understood this preparation is no longer needed.

If you want a conditional store, you might want to consider creating a new function and calling it. You might even be able to force it to be inlined.

I'll take this direction for now if inserting the new Function is safe here.

If the CFG of the assembly changes, that is ok.

I think we could introduce the new mode for TATAS (Test and Test and Set) to atomicrmw. So I will write the proposal if this is reasonable. I will introduce "conditional atomicrmw OR".