<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 22, 2014 at 4:35 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><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></div></blockquote><div><br></div><div>Yep, done.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+  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></blockquote><div><br></div><div>Turns out inline asm blobs have function types, so this "worked".  I'm going to reject musttail calls of inline asm, though, since we won't lower it to anything useful.  I don't think this is worth documenting in LangRef.  Let me know if you disagree.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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></div></blockquote><div><br></div><div>I approximated that by printing the operand of the call.  I don't see a way to print the index.  Ideally we'd caret the argument, but we might not have a .ll source file anyway.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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>
</div></div></blockquote><div><br></div><div>I like the existing wording because it explains *why* we have this rule.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>+  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></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Overall, this patch looks really good!</div>
</div></blockquote><div><br></div><div>Thanks, I'll commit and send you some inliner changes. </div></div></div></div>