<div dir="ltr"><div>Sorry, I've given up on phabricator today, it still won't let me add comments.</div><div><br></div><div><br></div><div>+static bool isTypeCongruent(Type *L, Type *R) {<br></div><div><br></div><div>

Function-level comment? "Congruency" of a type isn't a previously defined concept, you need to explain that this is testing the musttail compatibility property.</div>
<br>+  FunctionType *CalleeTy = GetFnTy(CI.getCalledValue());<div><br></div><div>What if the callee is musttail inline asm? Is this reachable for the invalid IR where the callee is not a function?</div><div><br></div><div>

+  for (int I = 0, E = CallerTy->getNumParams(); I != E; ++I) {<br>+    Assert1(<br>+        isTypeCongruent(CallerTy->getParamType(I), CalleeTy->getParamType(I)),<br>+        "cannot guarantee tail call due to mismatched parameter types", &CI);<br>

+  }<div class="gmail_extra"><br></div><div class="gmail_extra">Optional: include which n-th argument is mismatched?</div><div><br></div><div>As an aside, I'm don't like the wording "cannot guarantee tail call". The problem is that your IR doesn't match what LangRef requires, abstracted away from that other stuff. I don't have better wording, and it's *clear* as written.</div>

<br>+  Assert1(!Ret->getReturnValue() || Ret->getReturnValue() == RetVal,<br>+          "ret must return the result from a preceding musttail call", Ret);</div><div><br></div><div>I'd switch the object and subject, the rule isn't that a ret must return result from a musttail, it's that musttail's result must be returned.</div>

<div><br></div><div>Overall, this patch looks really good!</div><div><br></div><div>Nick</div><div><div class="gmail_extra"><br><div class="gmail_quote">On 9 April 2014 14:35, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">


<div>On Wed, Apr 9, 2014 at 2:24 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    For the LangRef, I suggest we remove the text related to when a
    "tail" call is guaranteed.  With the addition of "musttail" these
    semantics are redundant.  Instead, "tail" should be advisory only. 
    This would involve breaking backwards compatibility.<br>
    <br>
    With the new option, should GauranteedTailCallOpt exist?  I suggest
    not.  <br></div></blockquote><div><br></div></div><div>I agree, this feature obsoletes GTCO.  I don't think it's exposed via the C API, so we can probably just get rid of it.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">



<div bgcolor="#FFFFFF" text="#000000">
    In code, it would be useful to refer to the old "tail" attribute as
    "may tail".  This clarifies the semantics in code even if we don't
    change the IR name.  In particular, I'm referring to
    CallSite/Instruction here.<br>
    <br>
    In the verifier you need to handle chains of bitcasts.  These will
    reduce to a single bitcast, but InstCombine may not have run yet. 
    We don't want each opto pass to need to do this simplification.<br></div></blockquote><div><br></div></div><div>I don't think this is worth supporting.  The frontend should be able to handle this by avoiding such chains.  They already have to be careful about the other rules for musttail, so this isn't much burden.</div>


<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
    Otherwise, LGTM and I support the addition.  <br></div></blockquote><div><br></div></div><div>Thanks for the feedback!</div></div></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div></div>