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

In D76175#1923087 <https://reviews.llvm.org/D76175#1923087>, @jdoerfert wrote:

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

Um, right now 64 bit is only the pointer. How would one fit a pointer //and// 2 ints in 64 (or 32 bits).
Right now, if I understand correctly, the pointer and the `int` take about 12 bytes (96 bits). And probably 16 bytes because of the alignment.

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

Ok, good. I don't see right now how flow-sensitivity helps in IRP but I'll leave it for when time comes to introduce it.

> 
> 
>  ---
> 
> 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.
   };
 
----------------
jdoerfert wrote:
> FWIW. The encoding allows to check for arguments by doing `value >= 0`. If that is true, the argument number is the value. 
But how do you know if it's a call site argument or a formal parameter (i.e. `IRP_ARGUMENT`) ?
Since argument numbers start from 0, this is problematic (e.g. see the previous comment regarding the `argument()` constructor).
Plus then there are a lot of `switch`es that if say we have argument number == 3 don't seem to be that clear.
I'm mainly asking in order to be able to understand the current code so that I can change it.


================
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!");
----------------
jdoerfert wrote:
> 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)
Yes, ok, that's why I mentioned that its position seemed weird. I'll change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76175