[llvm-commits] [llvm] r151877 - in /llvm/trunk: lib/Target/X86/X86ISelLowering.cpp test/CodeGen/X86/sibcall-5.ll

[Evan Cheng]

On Mar 5, 2012, at 10:30 AM, Chad Rosier <mcrosier at apple.com> wrote:

> 
> On Mar 1, 2012, at 8:13 PM, Evan Cheng wrote:
> 
>> 
>> On Mar 1, 2012, at 6:50 PM, Chad Rosier wrote:
>> 
>>> 
>>> 
>>> +  // If anything is glued to the copy, then we can't safely perform a tail call.
>>> +  if (Copy->getOpcode() == ISD::CopyToReg &&
>>> +      Copy->getNumOperands() == 4)
>>> +    return false;
>> 
>> Hi Chad,
>> 
>> This is not the right way to check whether anything is glued to the copy. It's checking whether the copytoreg has a 4th argument and the use magic number is fragile. You want to check if the copytoreg has second value and whether that value has any use.
> 
> If I understand the comments in SelectionDAG.h correctly, the 4th operand is the glue value.  Thus, this seems to be the correct check.  I do agree, however, that using a magic number is error prone.  Is there a more generic (i.e., accessor function) for checking for glue?

Are you checking 1) something is glued to the copy? or 2) copy has a glue operand?

If it's #1, then you want to do something like Copy->hasAnyUseOfValue(1). If it's #2, then do Copy->getOperand(Copy->getNumOperands()-1).getValueType() == MVT::Glue (since glue has to be the last operand).

Evan

> 
> Here's the exact comment and relevant code:
> ----------
>  // This version of the getCopyToReg method takes an extra operand, which
>  // indicates that there is potentially an incoming glue value (if Glue is not
>  // null) and that there should be a glue result.
>  SDValue getCopyToReg(SDValue Chain, DebugLoc dl, unsigned Reg, SDValue N,
>                       SDValue Glue) {
>    SDVTList VTs = getVTList(MVT::Other, MVT::Glue);
>    SDValue Ops[] = { Chain, getRegister(Reg, N.getValueType()), N, Glue };
>    return getNode(ISD::CopyToReg, dl, VTs, Ops, Glue.getNode() ? 4 : 3);
>  }
> ----------
> 
>> 
>> Also since the code fragment before is:
>> 
>> if (Copy->getOpcode() != ISD::CopyToReg &&
>>     Copy->getOpcode() != ISD::FP_EXTEND)
>>   return false;
>> 
>> You want to write something like
>> 
>> if (Copy->getOpcode() != ISD::CopyToReg) {
> 
> Don't you mean:
> 
> if (Copy->getOpcode() == ISD::CopyToReg) {
>  // If anything is glued to the copy...
>  if (Glue)
>    return false;
> }
> 
> Specifically, this should be an equals comparison, not a not equal comparison.

Right.

Evan

> 
> Chad
> 
>> 	// If anything is glued to the copy ...
>> } else if (Copy->getOpcode() != ISD::FP_EXTEND)
>>   return false;
>> 
>> Evan
>> 
>>> +
>>> bool HasRet = false;
>>> for (SDNode::use_iterator UI = Copy->use_begin(), UE = Copy->use_end();
>>>      UI != UE; ++UI) {
>>> 
>>> Modified: llvm/trunk/test/CodeGen/X86/sibcall-5.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/sibcall-5.ll?rev=151877&r1=151876&r2=151877&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/CodeGen/X86/sibcall-5.ll (original)
>>> +++ llvm/trunk/test/CodeGen/X86/sibcall-5.ll Thu Mar  1 20:50:46 2012
>>> @@ -1,5 +1,6 @@
>>> ; RUN: llc < %s -mtriple=i386-apple-darwin8 -mattr=+sse2  | FileCheck %s --check-prefix=X32
>>> ; RUN: llc < %s -mtriple=x86_64-apple-darwin -mattr=+sse2 | FileCheck %s --check-prefix=X64
>>> +; RUN: llc < %s -mtriple=x86_64-apple-darwin -mattr=-sse3 | FileCheck %s --check-prefix=X64_BAD
>>> 
>>> ; Sibcall optimization of expanded libcalls.
>>> ; rdar://8707777
>>> @@ -29,3 +30,31 @@
>>> declare float @sinf(float) nounwind readonly
>>> 
>>> declare double @sin(double) nounwind readonly
>>> +
>>> +; rdar://10930395
>>> +%0 = type opaque
>>> +
>>> +@"\01L_OBJC_SELECTOR_REFERENCES_2" = external hidden global i8*, section "__DATA, __objc_selrefs, literal_pointers, no_dead_strip"
>>> +
>>> +define hidden { double, double } @foo2(%0* %self, i8* nocapture %_cmd) uwtable optsize ssp {
>>> +; X64_BAD: foo
>>> +; X64_BAD: call
>>> +; X64_BAD: call
>>> +; X64_BAD: call
>>> +  %1 = load i8** @"\01L_OBJC_SELECTOR_REFERENCES_2", align 8, !invariant.load !0
>>> +  %2 = bitcast %0* %self to i8*
>>> +  %3 = tail call { double, double } bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to { double, double } (i8*, i8*)*)(i8* %2, i8* %1) optsize
>>> +  %4 = extractvalue { double, double } %3, 0
>>> +  %5 = extractvalue { double, double } %3, 1
>>> +  %6 = tail call double @floor(double %4) optsize
>>> +  %7 = tail call double @floor(double %5) optsize
>>> +  %insert.i.i = insertvalue { double, double } undef, double %6, 0
>>> +  %insert5.i.i = insertvalue { double, double } %insert.i.i, double %7, 1
>>> +  ret { double, double } %insert5.i.i
>>> +}
>>> +
>>> +declare i8* @objc_msgSend(i8*, i8*, ...)
>>> +
>>> +declare double @floor(double) optsize
>>> +
>>> +!0 = metadata !{}
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>