This is an archive of the discontinued LLVM Phabricator instance.

[Argument Promotion] Only promote args when function attributes are compatible
ClosedPublic

Authored by tstellar on Oct 22 2018, 9:41 PM.

Details

Summary

Check to make sure that the caller and the callee have compatible
function arguments before promoting arguments. This uses the same
TargetTransformInfo queries that are used to determine if attributes
are compatible for inlining.

The goal here is to avoid breaking ABI when a called function's ABI
depends on a target feature that is not enabled in the caller.

This is a very conservative fix for PR37358. Ideally we would have a more
sophisticated check for ABI compatiblity rather than checking if the
attributes are compatible for inlining.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Oct 22 2018, 9:41 PM

@echristo wanted to look at this

echristo added inline comments.Nov 8 2018, 7:10 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
813 ↗(On Diff #170568)

Seems like we should expose this on TTI and that way backends can override and keep a single copy between here and InlineCost.cpp.

tstellar added inline comments.Nov 9 2018, 6:53 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
813 ↗(On Diff #170568)

These are the only callers of TTI.areInlineCompatible(). Should we just roll it into that or create something new like? Also, eventually won't we want different call-backs for InlineCost and ArgumentPromotion?

echristo added inline comments.Nov 12 2018, 2:30 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
813 ↗(On Diff #170568)

I'd be more inclined to go with a TTI::functionsHaveCompatibleAttributes that wraps the existing behavior.

And that's a good question. I think the answer is probably, but keep in mind that areInlineCompatible is already pulled out for X86.

Mostly I'd like to avoid the same static function duplicated - at that point let's just move the whole thing to TTI and then figure out the best way of specializing per target?

Sorry that this comment has gotten lost *twice*. I tried to write it over a week ago. =[[[[

lib/Transforms/IPO/ArgumentPromotion.cpp
813 ↗(On Diff #170568)

I would *strongly* advocate for keeping these completely separate all the way to the backend implementation... While these may happen to overlap today, they seem like deeply different concepts.

Also, see my comment below.

870–871 ↗(On Diff #170568)

I would sink this down and call it with the set of arguments so that it can filter out incompatible ones.

The backend really should have access to the arguments themselves before saying that things are incompatible. As an example, regardless of the attributes of the function, i32 arguments remain perfectly compatible and we should keep promoting those.

tstellar updated this revision to Diff 173787.Nov 12 2018, 5:06 PM

Add a new callback function areFunctionArgsABICompatible for the ArgumentPromotion
pass to use. @echristo I'm still a little confused about what to do with
functionsHaveCompatibleAttributes, should I just leave it as is, or use this new
callback?

echristo accepted this revision.Nov 13 2018, 4:24 PM

This looks better to me. I've got an inline request for some elaboration and you might want to let chandlerc comment, but otherwise OK with me.

include/llvm/Analysis/TargetTransformInfo.h
933 ↗(On Diff #173787)

"layout" is a bit difficult here, perhaps elaborate a bit?

This revision is now accepted and ready to land.Nov 13 2018, 4:24 PM
chandlerc requested changes to this revision.Nov 13 2018, 4:46 PM
chandlerc added inline comments.
include/llvm/Analysis/TargetTransformInfoImpl.h
533 ↗(On Diff #173787)

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.

This revision now requires changes to proceed.Nov 13 2018, 4:46 PM
tstellar added inline comments.Nov 13 2018, 7:08 PM
include/llvm/Analysis/TargetTransformInfoImpl.h
533 ↗(On Diff #173787)

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?

tstellar updated this revision to Diff 177999.Dec 12 2018, 8:35 PM

Some small updates:

+ Clarified Comment
+ Decoupled new function from areInlineCompatible()

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.

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

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

I'm also seeing issues from ArgumentPromotion with my min-legal-vector-width attribute used by X86 with avx512 enabled and -mprefer-vector-width=256. I think I'll need to override the X86 implementation of areFunctionArgsABICompatible to check for this attribute as well.

nikic added a subscriber: nikic.Dec 22 2018, 4:11 AM

Gentle ping. Any chance this is going to get fixed before 8.0 branches?

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

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.

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

chandlerc accepted this revision.Jan 15 2019, 8:31 PM

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.

include/llvm/Analysis/TargetTransformInfoImpl.h
533 ↗(On Diff #173787)

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.

This revision is now accepted and ready to land.Jan 15 2019, 8:31 PM
This revision was automatically updated to reflect the committed changes.