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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 5 inline comments as done.
sstefan1 added inline comments.


================
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;
+
----------------
jdoerfert wrote:
> This is "assumed" information, correct? We should put it in the name (or Known if that is what it is)
Makes sense.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2915
+      if (isMallocOrCallocLikeFn(I, TLI) || isFreeCall(I, TLI))
+        return ChangeStatus::CHANGED;
+
----------------
jdoerfert wrote:
> 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?
I had this in mind, but I don't think it would work because `isAssumedSideEffectFree()` will be false.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3053
+      return indicateOptimisticFixpoint();
+    }
+
----------------
jdoerfert wrote:
> 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.
default tracking was leftover actually. Changes in max iterations should've been a red flag.

If I'm not wrong, everything in here should be a `CallBase`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4928
+          return true;
+    }
+
----------------
jdoerfert wrote:
> 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?
Thanks for the suggestion. That indeed looks nicer :)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:7436
+
+        if (DeadValue || DeadCallSite)
+          continue;
----------------
jdoerfert wrote:
> 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`.
Actually it works. I should've taken a closer look at `IRPosition::value()`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72562