[PATCH] D65977: [Attributor] Use IRPosition consistently

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:158
+  static const IRPosition value(const Value &V, const Function &AnchorScope) {
+    // TODO: Determine if we need the scope at all.
+    if (auto *Arg = dyn_cast<Argument>(&V))
----------------
sstefan1 wrote:
> uenoku wrote:
> >  What are the advantages to record the scope? 
> I don't see the need for it.
We could then tie constants to functions, but again, I was also not sure why/if we want that.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:255
+      return *cast<Instruction>(V).getFunction();
+    // TODO: Figure out if this makes sense.
+    llvm_unreachable("Could not determine scope!");
----------------
sstefan1 wrote:
> You mean if `llvm_unreachable` makes sense? To me it does. If I'm not wrong this also covers CallSites and I don't see anything else that should be covered.
Yes, unreachable or nullptr was the question. And again, it is about constants (globals, constant integers, ...). It turns out, this is useful in the future so I use nullptr here.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:286
+    switch (getPositionKind()) {
+    case IRPosition::IRP_INVALID:
+    case IRPosition::IRP_FLOAT:
----------------
sstefan1 wrote:
> Maybe put these 2 in default with unreachable?
I would prefer to make it explicit.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:213
+
+  /// TODO
+  static const IRPosition function_scope(const IRPosition &IRP) {
----------------
uenoku wrote:
> What does this TODO mean?
> 
> 
It is a reminder to myself to describe the method ;). Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65977