[PATCH] D51650: Implement target_clones multiversioning

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:961-970
   if (const auto *FD = dyn_cast<FunctionDecl>(ND))
     if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
       if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion())
         AppendCPUSpecificCPUDispatchMangling(
             CGM, FD->getAttr<CPUSpecificAttr>(), Out);
+      else if (FD->isTargetClonesMultiVersion())
+        AppendTargetClonesMangling(CGM, FD->getAttr<TargetClonesAttr>(), Out);
----------------
erichkeane wrote:
> rsmith wrote:
> > This chain of `if`s and similar things elsewhere make me think we're missing an abstraction. Perhaps `FunctionDecl` should have a `getMultiVersionAttr()` and/or `getMultiVersionKind()` functions?
> I'm not super sure what either buys us... The multiversion attributes are all somewhat different unfortunately, so they would need to be dispatched separately later.  The 'getMultiVersionKind' is perhaps useful, though its essentially what isXXXMultiVersion does.  I'll think on it, I agree that there is likely an abstraction somewhere between that can improve this...
The idea would be that we have an enum that we can perform a covered `switch` over, so we don't need to remember to update all the relevant places when we add another kind of multiversioning. You already have the code to work out the kind of multiversioning; moving it from SemaDecl.cpp to FunctionDecl then using it where it makes sense in this patch would help, I think.


================
Comment at: lib/Sema/SemaDecl.cpp:9811-9812
 
   // Main isn't allowed to become a multiversion function, however it IS
   // permitted to have 'main' be marked with the 'target' optimization hint.
   if (NewFD->isMain()) {
----------------
erichkeane wrote:
> rsmith wrote:
> > Why can't `main` be multiversioned? That seems like an arbitrary restriction.
> At the time of implementing 'target', I was unsure of (and still am) how to accomplish this.  It would seem that I'd need to make the entry point a wrapper that calls the ifunc.  GCC seems to improperly call this (it doesn't emit the 'main' fucntion as far as I can tell).
OK, if there's actually a problem with having the `main` symbol be an ifunc, that seems like a perfectly legitimate reason for the restriction.


https://reviews.llvm.org/D51650