[PATCH] D92431: [SROA] Remove Dead Instructions while creating speculative instructions

[Arnamoy B via Phabricator via llvm-commits]

arnamoy10 created this revision.
arnamoy10 added reviewers: chandlerc, lebedev.ri, davide.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
arnamoy10 requested review of this revision.

The SROA pass tries to be lazy for removing dead instructions that are collected during iterative run of the pass in the DeadInsts list.  However it does not remove instructions from the dead list while running eraseFromParent() on those instructions.

This causes (rare) null pointer dereferences.  For example, in the speculatePHINodeLoads() instruction, in the following code snippet:

  while (!PN.use_empty()) {
    LoadInst *LI = cast<LoadInst>(PN.user_back());
    LI->replaceAllUsesWith(NewPN);
    LI->eraseFromParent();
  }

If the Load instruction LI belongs to the DeadInsts list, it should be removed when eraseFromParent() is called.  However, the bug does not show up in most cases, because immediately in the same function, a new LoadInst is created in the following line:

  LoadInst *Load = PredBuilder.CreateAlignedLoad(
           LoadTy, InVal, Alignment,
           (PN.getName() + ".sroa.speculate.load." + Pred->getName()));

This new LoadInst object takes the same memory address of the just deleted LI using eraseFromParent(), therefore the bug does not materialize.  In very rare cases, the addresses differ and therefore, a dangling pointer is created, causing a crash.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92431

Files:
  llvm/lib/Transforms/Scalar/SROA.cpp


Index: llvm/lib/Transforms/Scalar/SROA.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/SROA.cpp
+++ llvm/lib/Transforms/Scalar/SROA.cpp
@@ -1263,7 +1263,9 @@
   return true;
 }
 
-static void speculatePHINodeLoads(PHINode &PN) {
+typedef SetVector<Instruction *, SmallVector<Instruction *,8>> DeadInstsTy;
+
+static void speculatePHINodeLoads(PHINode &PN, DeadInstsTy &DeadInsts) {
   LLVM_DEBUG(dbgs() << "    original: " << PN << "\n");
 
   LoadInst *SomeLoad = cast<LoadInst>(PN.user_back());
@@ -1282,6 +1284,9 @@
   while (!PN.use_empty()) {
     LoadInst *LI = cast<LoadInst>(PN.user_back());
     LI->replaceAllUsesWith(NewPN);
+    // Remove from DeadInsts to avoid dangling pointers
+    if (DeadInsts.count(LI) > 0)
+      DeadInsts.remove(LI);
     LI->eraseFromParent();
   }
 
@@ -1354,7 +1359,7 @@
   return true;
 }
 
-static void speculateSelectInstLoads(SelectInst &SI) {
+static void speculateSelectInstLoads(SelectInst &SI, DeadInstsTy &DeadInsts) {
   LLVM_DEBUG(dbgs() << "    original: " << SI << "\n");
 
   IRBuilderTy IRB(&SI);
@@ -1365,6 +1370,13 @@
     LoadInst *LI = cast<LoadInst>(SI.user_back());
     assert(LI->isSimple() && "We only speculate simple loads");
 
+    // Do not have to process DEAD Loads
+    if (DeadInsts.count(LI) > 0) {
+      DeadInsts.remove(LI);
+      LI->eraseFromParent();
+      continue;
+    }
+
     IRB.SetInsertPoint(LI);
     LoadInst *TL = IRB.CreateLoad(LI->getType(), TV,
                                   LI->getName() + ".sroa.speculate.load.true");
@@ -4664,11 +4676,11 @@
 
   LLVM_DEBUG(dbgs() << "  Speculating PHIs\n");
   while (!SpeculatablePHIs.empty())
-    speculatePHINodeLoads(*SpeculatablePHIs.pop_back_val());
+    speculatePHINodeLoads(*SpeculatablePHIs.pop_back_val(), DeadInsts);
 
   LLVM_DEBUG(dbgs() << "  Speculating Selects\n");
   while (!SpeculatableSelects.empty())
-    speculateSelectInstLoads(*SpeculatableSelects.pop_back_val());
+    speculateSelectInstLoads(*SpeculatableSelects.pop_back_val(), DeadInsts);
 
   return Changed;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92431.308756.patch
Type: text/x-patch
Size: 2090 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201201/446e4cec/attachment.bin>