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

> sizeof(Kind + ArgNo) <= 64

Oh bytes, yes. With the change they will take 16 bytes (as now).

@sstefan1 
Thanks for accepting, I'll upload another diff and I'll leave other bigger changes (like the kind thing or the `getAnchorScope()` - if it gets agreed upon - for followups).



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:168
     IRP_CALL_SITE_ARGUMENT = 1, ///< An attribute for a call site argument.
   };
 
----------------
jdoerfert wrote:
> baziotis wrote:
> > 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.
> If the value is smaller than 0, it is of the type specified by the value, thus the value is the "kind".
> If the value is at least 0 and the associated value is an llvm::Argument, it is an argument at position value.
> If the value is at least 0 and the associated value is an llvm::CallBase, it is a call site argument at position value.
Oh ok, using `dyn_cast` or sth on the `AnchorVal`. Thanks, I thought it was somehow done only with `KindOrArgNo` and I missed it.


================
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:
> jdoerfert wrote:
> > baziotis wrote:
> > > 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.
> > These are different things for call site [arguments]
> I agree, my bad. I think that there are at least few places that we use `getAssociatedFunction` instead of `getAnchorScope`. Not necessary in this patch.
So, yesterday I saw quite late this `getCalledFunction()` which should create problems and it did. I left it for today to try to combine them (in `getAssociatedFunction()`) better but it seems we can't. Anytime that we do CGSCC pass for example, for `call` instructions is different (or when we get scope for `AAValueRange`).
It may actually be a good idea to decouple them further rather than combining them. Specifically:
> I think that there are at least few places that we use getAssociatedFunction instead of getAnchorScope
to me this is a problem because `getAssociatedFunction()` does not have a coherent behavior. It's like "I'll give you the parent function most of the time, except if the anchor value is a CallBase". So, when you see it used in the code, it's unclear whether the one who wrote it thought:
- I know the anchor value is a CallBase, I expect the callee back.
- I know it's not, I want the parent function of the AnchorVal.
- I want a whatever scope that should be a function.

I think:
- Case 1 calls should be left as they are.
- Case 2 calls should be replaced with getAnchorScope
- Case 3 calls should be eliminated because they're unclear.

Plus, if that happens, we should change `getAssociatedFunction()` to only return the callee and otherwise null (i.e. remove the ambiguity).

See follow-up diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76175