<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018, 2:38 PM Chandler Carruth via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added a comment.<br>
<br>
Sorry that this comment has gotten lost *twice*. I tried to write it over a week ago. =[[[[<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/IPO/ArgumentPromotion.cpp:813<br>
+/// Test that there are no attribute conflicts between Caller and Callee<br>
+static bool functionsHaveCompatibleAttributes(const CallSite &CS,<br>
+                                              const TargetTransformInfo &TTI) {<br>
----------------<br>
echristo wrote:<br>
> tstellar wrote:<br>
> > echristo wrote:<br>
> > > Seems like we should expose this on TTI and that way backends can override and keep a single copy between here and InlineCost.cpp.<br>
> > 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?<br>
> I'd be more inclined to go with a TTI::functionsHaveCompatibleAttributes that wraps the existing behavior.<br>
> <br>
> And that's a good question. I think the answer is probably, but keep in mind that areInlineCompatible is already pulled out for X86.<br>
> <br>
> 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?<br>
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.<br></blockquote></div><div><br></div><div><br></div><div>That's absolutely ok with me. </div><div><br></div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, see my comment below.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/IPO/ArgumentPromotion.cpp:870-871<br>
<br>
+    if (!functionsHaveCompatibleAttributes(CS, TTI))<br>
+      return nullptr;<br>
+<br>
----------------<br>
I  would sink this down and call it with the set of arguments so that it can filter out incompatible ones.<br>
<br>
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.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D53554" rel="noreferrer" target="_blank">https://reviews.llvm.org/D53554</a><br>
<br>
<br>
<br>
</blockquote></div>