[PATCH] D72562: [Attributor][Fix] AAHeapToStack and AAIsDead connection

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This is basically what I was hoping for, thx for rebasing it on the new scheme. Some comments inlined.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2463
+  /// Returns true if instruction (malloc/free) is identified for deletion.
+  virtual bool isToBeDeleted (Attributor &A, Instruction *I) const;
+
----------------
This is "assumed" information, correct? We should put it in the name (or Known if that is what it is)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2915
+      if (isMallocOrCallocLikeFn(I, TLI) || isFreeCall(I, TLI))
+        return ChangeStatus::CHANGED;
+
----------------
Would it work the other way around, as well? Deleting them here not in H2S. If so it would remove the special case here and some code in H2S, right?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3053
+      return indicateOptimisticFixpoint();
+    }
+
----------------
Do not track dependences by default but manually register one in the conditional when H2S information is used.

----

the entire H2s thing should be in an scope like:
```
} else if (isa<CallBase>(CtxI)) {
<h2s>
}
```
to avoid calling it if we know its side effect free or not a call.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4928
+          return true;
+    }
+
----------------
I think we can just iterate over all of them with some helpers to keep the code simpler:

```
auto IsI = [I](Instruction *C) { return C == I; };
if (llvm::any_of(MallocCalls, IsI))
  ...
for (auto &It : FreesForMalloc)
  if (llvm::any_of(It.second, IsI))
    ...
```
wdyt?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:7436
+
+        if (DeadValue || DeadCallSite)
+          continue;
----------------
I would hope the existing single `Attributor::isAssumedDead` check would suffice. The problem is we will play whack-a-mole all over the place if we cannot "generalize" liveness queries.

Can you explain why it's not. What would it take to make it work with a single `Attributor::isAssumedDead` call here, moving the logic into `Attributor::isAssumedDead`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72562