[PATCH] Add 'musttail' marker to call instructions

[Nick Lewycky]

Sorry, I've given up on phabricator today, it still won't let me add
comments.


+static bool isTypeCongruent(Type *L, Type *R) {

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.

+  FunctionType *CalleeTy = GetFnTy(CI.getCalledValue());

What if the callee is musttail inline asm? Is this reachable for the
invalid IR where the callee is not a function?

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

Optional: include which n-th argument is mismatched?

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.

+  Assert1(!Ret->getReturnValue() || Ret->getReturnValue() == RetVal,
+          "ret must return the result from a preceding musttail call",
Ret);

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.

Overall, this patch looks really good!

Nick

On 9 April 2014 14:35, Reid Kleckner <rnk at google.com> wrote:

> On Wed, Apr 9, 2014 at 2:24 PM, Philip Reames <listmail at philipreames.com>wrote:
>
>>  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.
>>
>> With the new option, should GauranteedTailCallOpt exist?  I suggest not.
>>
>
> 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.
>
>
>>  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.
>>
>> 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.
>>
>
> 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.
>
>
>> Otherwise, LGTM and I support the addition.
>>
>
> Thanks for the feedback!
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140422/34ce5b89/attachment.html>