[PATCH] D85544: [OpenMPOpt] ICV tracking for calls

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D85544#2211355 <https://reviews.llvm.org/D85544#2211355>, @sstefan1 wrote:

> Unclear if we still want  D85052 <https://reviews.llvm.org/D85052> to go in before this, since most of the login is replaced with this patch.
>
> Should I just merge the good parts with this patch?

Yes I think so.

TBH, I'm thinking we should embrace the IRPositions and Attributor design fully, as it seems a bit messy right now (= we reason about returns, calls, and instructions all somewhat together).

- `AAICVTrackerFunction` for the function body, given an ICV and an instruction, what is the value by backwards lookup (as you have it now). When you hit a call, ask `AAICVTrackerCallSiteReturn`
- `AAICVTrackerFunctionReturn` which should forward queries to `AAICVTrackerFunction` if it is not invalid. The queries are done for all return inst.
- `AAICVTrackerCallSiteReturn` which should forward queries to `AAICVTrackerFunctionReturn`.

WDYT?



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1142
+                                                      const Instruction *I,
+                                                      Attributor &A) const = 0;
+
----------------
I am unsure about the difference of these two functions from the declarations. If we need two member functions we should name them appropriately and elaborate in the description.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1183
+  virtual Optional<Value *> getUniqueValue(InternalControlVar ICV,
+                                           Attributor &A) const = 0;
 
----------------
jdoerfert wrote:
> The comment of `getUniqueValue` is not helpful. It looks like this will compute the value of `ICV` after the function was called, correct? Whatever it does, write it here ;)
The description is now better, we should also change the name to match it. Maybe `getICVValueAfterOnReturn` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85544