[PATCH] D65977: [Attributor] Use IRPosition consistently

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 added a comment.

Generally looks good to me. Some initial comments inlined. Since it is a big patch I'd also wait for Hideto to read it. (Maybe I'll reread it as well.)



================
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))
----------------
I don't see the need for it.


================
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!");
----------------
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.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:286
+    switch (getPositionKind()) {
+    case IRPosition::IRP_INVALID:
+    case IRPosition::IRP_FLOAT:
----------------
Maybe put these 2 in default with unreachable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65977