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

[Eric Christopher via llvm-commits]

On Mon, Nov 12, 2018, 2:38 PM Chandler Carruth via Phabricator <
reviews at reviews.llvm.org> wrote:

> chandlerc added a comment.
>
> Sorry that this comment has gotten lost *twice*. I tried to write it over
> a week ago. =[[[[
>
>
>
> ================
> Comment at: lib/Transforms/IPO/ArgumentPromotion.cpp:813
> +/// Test that there are no attribute conflicts between Caller and Callee
> +static bool functionsHaveCompatibleAttributes(const CallSite &CS,
> +                                              const TargetTransformInfo
> &TTI) {
> ----------------
> echristo wrote:
> > tstellar wrote:
> > > echristo wrote:
> > > > Seems like we should expose this on TTI and that way backends can
> override and keep a single copy between here and InlineCost.cpp.
> > > 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?
> > 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?
> 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.
>


That's absolutely ok with me.



> Also, see my comment below.
>
>
> ================
> Comment at: lib/Transforms/IPO/ArgumentPromotion.cpp:870-871
>
> +    if (!functionsHaveCompatibleAttributes(CS, TTI))
> +      return nullptr;
> +
> ----------------
> 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.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D53554
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181112/91269730/attachment.html>