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

[Chandler Carruth via Phabricator via llvm-commits]

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.

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