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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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.


Great! It's about time we give it a overhaul, a lot of the documentation is still from the very beginning as well. Feel free
to continue issuing design suggestions.

> 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. Plus, `IRP` currently
>  saves a pointer (`Value *`, the anchor value) and an `int`. For most users of LLVM, a pointer will take 8 bytes. Because of alignment,
>  we will use another 8 bytes anyway for `KindOrArgNo`, not 4. And I think it will be so much simpler to use the wasted 4 bytes
>  to decouple the values. That is, one `int` for the kind and one `int`for the argument number.
> 
> If we all agree in that, of course I'm willing to do the related patches.

You can split the two apart but fit them into at most 64bit, maybe even 32.
FWIW, I was hoping to add a position (=Instruction) to this class (or a subclass) at some point so we can do flow sensitive queries.

---

I'm fine with this. @sstefan1 please accept once you are satisfied.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:168
     IRP_CALL_SITE_ARGUMENT = 1, ///< An attribute for a call site argument.
   };
 
----------------
FWIW. The encoding allows to check for arguments by doing `value >= 0`. If that is true, the argument number is the value. 


================
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!");
----------------
baziotis wrote:
> 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.
not redundant but should be at the start. If it's invalid we crash on the dyn_cast otherwise (or worse)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76175