[PATCH] D121419: [WPD] Extend checking mode to support fallback to indirect call

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D121419#3374225 <https://reviews.llvm.org/D121419#3374225>, @luna wrote:

> It occurred to me that the WPD internal doc also mentions `--wholeprogramdevirt-check`, and current example uses it as a boolean.

Yep, I'll be updating the internal doc - afaik no one is using it (was previously just used by me for debugging).



================
Comment at: llvm/include/llvm/Transforms/Utils/CallPromotionUtils.h:84
+/// returned.
+CallBase &versionCallSite(CallBase &CB, Value *Callee, MDNode *BranchWeights);
+
----------------
luna wrote:
> nit pick
> 
> I'm wondering if it's more conventional to keep examples [1] in cpp or in the header (both are pretty readable IMO)
> 
> [1] https://github.com/llvm/llvm-project/blob/fc968bcba4d7e02390f1c1cba76a6a2cad1ae59f/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp#L195-L281
Not sure of the convention, but I wanted to keep the comment block in the header reasonable and similar in size to the existing ones. The examples comment block in the source file is really large for this function. I'll keep it there for now.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1174
+        CallBase &NewInst = versionCallSite(CB, Callee, Weights);
+        NewInst.setCalledOperand(Callee);
+      }
----------------
luna wrote:
> `llvm::promoteCall` [1] clears two types of metadata and then compares function type.
> 
> Function types are the same (for virtual functions); I'm wondering if it's needed to clear two types of metadata here (for my understanding)
> 
> [1] https://github.com/llvm/llvm-project/blob/fc968bcba4d7e02390f1c1cba76a6a2cad1ae59f/llvm/lib/Transforms/Utils/CallPromotionUtils.cpp#L456
Good catch. Looks like leaving this metadata on direct calls doesn't cause any issues, since it hasn't been stripped for the normal WPD (non-fallback) case before. But it causes a real issue when left on the fallback indirect case in the new handling, because ICP will come along and promote again. This isn't a correctness issue, but could bloat codesize - although in the case I tested the optimizer smartly merged the redundant promoted direct calls. But I have fixed it in all cases (fallback and for the direct calls under all options) and added tests for all of these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121419