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

[Chad Rosier]

On Mar 5, 2012, at 10:50 AM, Evan Cheng wrote:

> 
> 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?

I'm checking the latter.  Thanks for the suggestion.  I'll update the comments and code accordingly.

> 
> 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
>>> 
>> 
>