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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Sorry, for some reason this didn't update as ready for another round of review.

In D53554#1329303 <https://reviews.llvm.org/D53554#1329303>, @tstellar wrote:

> Some small updates:
>
> + Clarified Comment
>  + Decoupled new function from areInlineCompatible()


Nice.

> Questions I have still:
> 
> + 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.

> + Do we need a separate function byval and normal args?  I thought that information
>  was accessible via the Argument objects that are passed to the function.

I'm fine with whatever works here -- as long as we can model what we need to.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53554