[PATCH] D76175: [Attributor][NFC] Typos in doc

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked 2 inline comments as done.
baziotis added a comment.

> I kind of like how this is working, but if others agree you can change it.

I appreciate it, it may be that I was not involved when it was written. It just seems
to be unclear how it's working. Consider this:

  /// Create a position describing the argument \p Arg.
  static const IRPosition argument(const Argument &Arg) {
    return IRPosition(const_cast<Argument &>(Arg), Kind(Arg.getArgNo()));
  }

`Arg.getArgNo()` can be 1, in which case, the respective `Kind` is `IRP_CALL_SITE_ARGUMENT`. At least to me, it makes
no sense why we want that. Again, it might be that only //I// don't get it but I have a lot of "why this happens?" moments
when reading different parts of the Attributor that have to do with arguments.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:278
       return CB->getCalledFunction();
     assert(KindOrArgNo != IRP_INVALID &&
            "Invalid position does not have an anchor scope!");
----------------
sstefan1 wrote:
> I think this assert is also redundant, given that it is called again in `getAnchorValue()`
Yes, although I don't understand why it's not at the start of this function.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:280
            "Invalid position does not have an anchor scope!");
-    Value &V = getAnchorValue();
-    if (isa<Function>(V))
-      return &cast<Function>(V);
-    if (isa<Argument>(V))
-      return cast<Argument>(V).getParent();
-    if (isa<Instruction>(V))
-      return cast<Instruction>(V).getFunction();
-    return nullptr;
+    return getAnchorScope();
   }
----------------
sstefan1 wrote:
> This makes sense. Now that I look at the other parts we seem to mix these two a lot. I think the better solution is to inline `getAnchorScope` here and replace all the call sites with `getAssociatedFunction` as it's clearer (at least to me). 
Yes, I like that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76175