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

[Mingming Liu via Phabricator via llvm-commits]

luna accepted this revision.
luna added a comment.

In D121419#3375351 <https://reviews.llvm.org/D121419#3375351>, @tejohnson wrote:

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

Thanks for making the change!



================
Comment at: llvm/include/llvm/Transforms/Utils/CallPromotionUtils.h:84
+/// returned.
+CallBase &versionCallSite(CallBase &CB, Value *Callee, MDNode *BranchWeights);
+
----------------
tejohnson wrote:
> 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.
> 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.

Yeah the current way is reasonable and readable, so sounds good to keep it as is.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1174
+        CallBase &NewInst = versionCallSite(CB, Callee, Weights);
+        NewInst.setCalledOperand(Callee);
+      }
----------------
tejohnson wrote:
> 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.
thanks for figuring it out and making the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121419