[PATCH] D53554: [Argument Promotion] Only promote args when function attributes are compatible

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Sorry I misunderstood that you were saying BasicTTI already had all the logic you were adding.

I *am* thinking about getting promotion to continue to work for obviously safe scalars even when the subtargets differ slightly. As we get more and more of these inferred from the use of instrinsics, I think this will be fairly important as I explain below. Doesn't need to be this patch though so LGTM if you want to do that as a follow-up.



================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:533
+    return areInlineCompatible(Caller, Callee) &&
+           AttributeFuncs::areInlineCompatible(*Caller, *Callee);
+  }
----------------
tstellar wrote:
> chandlerc wrote:
> > While this may be sufficient, it isn't necessary and seems a but of a conflation...
> > 
> > I'd much rather this be sunk into the codegen layers basic TTI impl so that targets can override it cleanly.
> > 
> > Also, I don't think byval and normal arguments have the same constraints.
> > 
> > Also, I'd really like to ensure that basic integers that are valid to promote across subtargets still get promoted even when inlining wouldn't be valid.
> Ok, so are you suggesting that I move this function definition into BasicTTIImplBase and re-implement it to handle the simple case of basic integers?  Anything else?
> 
> 
> 
> 
I guess this is fine for NoTTI.

It would seem nice to teach BasicTTI to handle some extremely common cases (scalar types that are legal in both caller and callee subtargets) so that we don't lose much in the way of real-world optimization.

I'm OK if that's a second patch, but I'd really like to have both of them so we don't have bizarre performance cliffs where you use a fancy new intrinsic for a specialized CPU instruction and regress the basic optimization of the code.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53554