[PATCH] D65408: [Attributor] Heap-To-Stack Conversion

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

We need tests where stuff is read/written as well.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2719
+  for (Instruction *DeadMalloc : DeadMallocs)
+    MallocCalls.remove(const_cast<Instruction *>(DeadMalloc));
+
----------------
Mallocs can become live later so we shouldn't remove them but only ignore them for the moment. In fact, the `checkForAllCallLikeInstructions` call will already filter "currently assumed dead" ones out so you have to invoke it in every update or collect all calls through the InfoCache explicitly in the initialize method.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2724
+      if (MallocCall == &I)
+        continue;
+
----------------
This is quadratic in the number of malloc calls.

If you want to exclude malloc calls from the tests below do:
`if (MallocCalls.count(&I)) return true;`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2729
+          if (Ptr == MallocCall) {
+            FreesForMalloc[const_cast<Instruction *>(MallocCall)].insert(&I);
+            return true;
----------------
No need to check, just put it in `FreesForMalloc` (though I'm unclear why we need it at the moment)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2773
+
+  A.checkForAllCallLikeInstructions(CallCheck, *this);
+
----------------
I was hoping we follow and check uses:

```
UsesCheck = (Instruction &I) {
  SmallPtrSet<const Use *, 8> Visisted;
  SmallVector<const Use *, 8> Worklist;
  Worklist.append(I.use_begin(), I.use_end());
  while (!Worklist.empty()) {
    const Use *U = Worklist.pop_back_val();
    if (!Visited.insert(U).second) continue;
    const auto *UserI = U->getUser();
    if (isa<LoadInst>(UserI) || isa<StoreInst>(UserI))
     continue;
    if (auto *CB = dyn_cast<CallBase>(UserI)) {
      // TODO: Check use once we have nofree on call site args.
      const auto &NoFreeAA = A.getAAFor<AANoFree>(..., IRPosition::call_site(CB));
      if (!NoFreeAA.isAssumedNoFree()) return false;
      // same for nocapture
      // if both are good, continue.
    }
    if (isa<GetElementOperator>(UserI) || isa<BitCastInst>(UserI)) {
      // Add users to worklist and continue
    }
    // unknown user
    return false;
  }
  return true;
}

MalloCheck = (Instruction &I) {
  if (!isaMalloc(I)) return true;
  if (BadMallocs.count(&I)) return true;
  // assumed h2s malloc I
  if (UsesCheck(I))
    Mallocs.insert(&I);
  else
    BadMallocs.insert(&I);
  return true;
}

checkForAllCallLikeInstructions(MallocCheck)
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2787
+    STATS_DECL(MallocCalls, Function,
+               "Number of MallocCalls");
+    BUILD_STAT_NAME(MallocCalls, Function) += MallocCalls.size();
----------------
Is this "Number of mallocs converted to allocs"?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2792
+
+using AAHeapToStackCallSite = AAHeapToStackFunction;
+
----------------
I doubt it makes sense to have a H2S call site AA. We should probably only allow the function one.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3213
+  if (EnableHeapToStack)
+    checkAndRegisterAA<AAHeapToStackFunction>(FPos, *this, nullptr);
+
----------------
Still use the Whitelist and group it with the other function AAs please


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:40
+  ret void
+}
+
----------------
We need a test, `sync_will_return` before `free` which should (later) be OK.


================
Comment at: llvm/test/Transforms/FunctionAttrs/heap_to_stack.ll:120
+  ret void
+}
----------------
This should be h2s as the free is dead and there are no users that are nofree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65408/new/

https://reviews.llvm.org/D65408