[PATCH] D52894: Update CallSite docs and add a new function (NFC)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/IR/CallSite.h:126
+  /// same function. For example, a call to a bitcast of a function is
+  /// considered indirect.
   bool isIndirectCall() const {
----------------
scott.linder wrote:
> efriedma wrote:
> > scott.linder wrote:
> > > efriedma wrote:
> > > > This isn't what isIndirectCall actually implements...
> > > Right, I completely missed that a BitCast is a Constant, and also did not mention the specific check for InlineAsm.
> > > 
> > > When can `getCalledValue` return `nullptr`? And is this definition "correct" for LLVM (e.g. an indirect call is any call which is not one of these cases)?
> > getCalledValue never returns nullptr.
> > 
> > Assuming you meant getCalledFunction, it only returns non-null if the callee is a Function.
> > 
> > isIndirectCall() isn't really used by anything outside of profiling infrastructure, so the implementation doesn't really affect much.
> > 
> > Maybe it's best to try to avoid defining "direct" and "indirect" calls, and just directly describe what the functions return.
> I meant getCalledValue; the first check in isIndirectCall is:
> 
> ```
> const Value *V = getCalledValue();
> if (!V)
>       return false;
> ```
> 
> Which is redundant if getCalledValue can't return nullptr.
> 
> The point of this patch is to clarify the definition, so having a function called `isIndirectCall` which answers a completely different question doesn't really move anything forward. I don't have a good grip on the right way to define this, though, as the term gets used in a number of places in a way which implies it has multiple definitions. I think ideally "indirect" would actually be defined as it is implemented here, for `isIndirectCall`, because as far as I know there is no way to introduce a non-constant value into a Constant expression.
> Which is redundant if getCalledValue can't return nullptr.

Yes, it's redundant.

-----

isIndirectCall() doesn't really implement what anything in LLVM treats as an indirect call outside of the profiling infrastructure; it should probably be renamed.


https://reviews.llvm.org/D52894