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

[Tom Stellard via Phabricator via llvm-commits]

tstellar added a comment.

In D53554#1350691 <https://reviews.llvm.org/D53554#1350691>, @chandlerc wrote:

> In D53554#1329303 <https://reviews.llvm.org/D53554#1329303>, @tstellar wrote:
>
> > + I'm still a little confused by the layering of TTI.  I tried to move the
> >  implementation out of TargetTransformInfoImplBase and into BasicTTI, but
> >  that breaks the NoTTIImpl class.
>
>
> For `NoTTIImpl` you can just return `false`, or return `false` except for integer types that DataLayout thinks are legal.
>
> The difference between `NoTTIImpl` and the "basic" layer inside CodeGen is that the basic layer can reach into common target-independent parts of the code generator (like the type legalization tables) to compute plausible answers, where `NoTTIImpl` is supposed to only return trivially evident information based on the IR-level input. Further, there is *no* quality bar for `NoTTIImpl` -- it's more for target-indepndent testing, and not supposed to have any quality per-se.


Ok, so as I understand, my patch implements the same default for both NoTTI and
BasicTTI since TargetTransformInfoImplBase is the base class for both.    Are you
OK with keeping this default implementation I have for NoTTI?  The advantage of 
my implementation is that it keeps ArugmentPromotion working for 'normal' C/C++ 
code and only disables it for code with functions targeting different CPU features in the same object.

>> + Is it possible to implement a generic version of this function for simple argument
>>  types without any target-specifc knowledge?  Does LLVM IR guarantee anything about
>>  calling convention for simple types?
> 
> It is possible to implement (i suggest one above) but I don't think there is any guaranteed ABI at the IR level. Maaaaybe inside the code generator there are some common rules that can handle easy cases, but I think we'll have to *establish* these rules (and check that the existing targets uphold them), not rely on them already being there.

Given that we are close to the branch point and this fixes at least one bug, are we OK to commit this
patch as is and then try to work out the IR level guarantees in a future patch?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53554