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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 added a comment.

In D76175#1922874 <https://reviews.llvm.org/D76175#1922874>, @baziotis wrote:

> Today I started reading through the whole Attributor, or at least its core parts, because I think otherwise I won't
>  have a good enough understanding to proceed. This is the start of a series of small patches that will mostly be NFC
>  improvements to docs with which I hope I'll have the opportunity to ask questions about different decisions in the Attributor.


This seems fine, as well as the initiative. Few comments though

> Starting off, I tried multiple times to read `IRPosition` and in my humble opinion, it seems //way// too complicated.
>  Specifically the `KindOrArgNo`. This coupling of basically 2 kinds of info in one seems unnecessary.

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



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:38
 // The update method `AbstractAttribute::updateImpl` is implemented by the
-// specific "abstract attribute" subclasses. The method is invoked whenever the
-// currently assumed state (see the AbstractState class) might not be valid
+// specific "abstract attribute"(AA*) subclasses. The method is invoked whenever
+// the currently assumed state (see the AbstractState class) might not be valid
----------------
don't think this is necessary.


================
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!");
----------------
I think this assert is also redundant, given that it is called again in `getAnchorValue()`


================
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();
   }
----------------
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). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76175