r201439 - [CodeGenPrepare][AddressingModeMatcher] Give up on type promotion if the

Wed Feb 19 11:58:41 PST 2014

On Feb 19, 2014, at 11:51 AM, Tom Stellard <tom at stellard.net> wrote:

> On Wed, Feb 19, 2014 at 11:29:43AM -0800, Quentin Colombet wrote:
>> Hi Tom,
>> 
>> That’s weird that bugpoint is linking correctly for you.
> 
> I just realized that it's probably working for me because I
> build with --enable-shared.
That could explain!

> 
>> 
>> I have dug in that linking problem and here is what I found.
>> Because of the CodeGenPrepare pass, libLLVMScalarOpts has a dependency on libLLVMCodeGen, which is not included on the link command of bugpoint.
>> 
>> So far it was working because all the functions of TargetLowering called in CodeGenPrepare are either:
>> - virtual function (resolved dynamically).
>> - inlined in the header (code available statically).
>> 
>> InstructionOpcodeToISD is not a virtual function and not inlined in the header, thus the linking problem.
>> 
>> We could add a dependency in bugpoint on libLLVMCodeGen, but I am not sure we want to do that.
> 
> This seems to me like the best solution.  What are the disadvantages to doing this?
The problem is possibly every consumer of libLLVMScalarOpts would also have to link against libLLVMCodeGen.

Although this seems to be a hack, one alternative would be to declare this specific function as virtual.
Rafael created the same problem in one of his recent commit, see http://llvm.org/bugs/show_bug.cgi?id=18900.

I will see if he can think of a better way of fixing this problem and I would apply the same fix.

-Quentin 

> 
> -Tom
> 
>> Another possibility would be to inline InstructionOpcodeToISD into the header, though I haven’t check how much this is pulling into the header.
>> 
>> Thoughts?
>> 
>> Cheers,
>> -Quentin
>> 
>> On Feb 18, 2014, at 2:28 PM, Tom Stellard <tom at stellard.net> wrote:
>> 
>>> On Tue, Feb 18, 2014 at 12:45:33PM -0800, Quentin Colombet wrote:
>>>> 
>>>> On Feb 18, 2014, at 11:19 AM, Tom Stellard <tom at stellard.net> wrote:
>>>> 
>>>>> On Tue, Feb 18, 2014 at 11:00:05AM -0800, Quentin Colombet wrote:
>>>>>> 
>>>>>> On Feb 18, 2014, at 10:30 AM, Tom Stellard <tom at stellard.net> wrote:
>>>>>> 
>>>>>>> On Tue, Feb 18, 2014 at 09:47:21AM -0800, Quentin Colombet wrote:
>>>>>>>> 
>>>>>>>> On Feb 18, 2014, at 9:37 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>>>> 
>>>>>>>>> Hi Tom,
>>>>>>>>> 
>>>>>>>>> On Feb 18, 2014, at 9:21 AM, Tom Stellard <tom at stellard.net> wrote:
>>>>>>>>> 
>>>>>>>>>> Hi Quentin,
>>>>>>>>>> 
>>>>>>>>>> Sorry I missed this on the first time around: 
>>>>>>>>>> 
>>>>>>>>>>> --- a/lib/Transforms/Scalar/CodeGenPrepare.cpp
>>>>>>>>>>> +++ b/lib/Transforms/Scalar/CodeGenPrepare.cpp
>>>>>>>>>>> @@ -1728,6 +1730,35 @@
>>>>>>>>>>> TypePromotionHelper::promoteOperandForOther(Instruction *SExt,
>>>>>>>>>>> return SExtOpnd;
>>>>>>>>>>> }
>>>>>>>>>>> 
>>>>>>>>>>> +/// IsPromotionProfitable - Check whether or not promoting an
>>>>>>>>>>> instruction
>>>>>>>>>>> +/// to a wider type was profitable.
>>>>>>>>>>> +/// \p MatchedSize gives the number of instructions that have been
>>>>>>>>>>> matched
>>>>>>>>>>> +/// in the addressing mode after the promotion was applied.
>>>>>>>>>>> +/// \p SizeWithPromotion gives the number of created instructions for
>>>>>>>>>>> +/// the promotion plus the number of instructions that have been
>>>>>>>>>>> +/// matched in the addressing mode before the promotion.
>>>>>>>>>>> +/// \p PromotedOperand is the value that has been promoted.
>>>>>>>>>>> +/// \return True if the promotion is profitable, false otherwise.
>>>>>>>>>>> +bool
>>>>>>>>>>> +AddressingModeMatcher::IsPromotionProfitable(unsigned MatchedSize,
>>>>>>>>>>> +                                             unsigned SizeWithPromotion,
>>>>>>>>>>> +                                             Value *PromotedOperand) const {
>>>>>>>>>>> +  // We folded less instructions than what we created to promote the operand.
>>>>>>>>>>> +  // This is not profitable.
>>>>>>>>>>> +  if (MatchedSize < SizeWithPromotion)
>>>>>>>>>>> +    return false;
>>>>>>>>>>> +  if (MatchedSize > SizeWithPromotion)
>>>>>>>>>>> +    return true;
>>>>>>>>>>> +  // The promotion is neutral but it may help folding the signextension in
>>>>>>>>>>> +  // loads for instance.
>>>>>>>>>>> +  // Check that we did not create an illegal instruction.
>>>>>>>>>>> +  Instruction *PromotedInst = dyn_cast<Instruction>(PromotedOperand);
>>>>>>>>>>> +  if (!PromotedInst)
>>>>>>>>>>> +    return false;
>>>>>>>>>>> +  return TLI.isOperationLegalOrCustom(PromotedInst->getOpcode(),
>>>>>>>>>>> +                                      EVT::getEVT(PromotedInst->getType()));
>>>>>>>>>> 
>>>>>>>>>> This won't work, because the IR Instructions and SelectionDAG Opcodes
>>>>>>>>>> have different enumeration values.
>>>>>>>>> Right, good catch!
>>>>>>>>> 
>>>>>>>>>> The original test case must have
>>>>>>>>>> passed, because it is using a default TargetLowering implementation
>>>>>>>>>> rather than the one from R600.
>>>>>>>>>> 
>>>>>>>>>> I've attached an updated version of the original test case which compiles
>>>>>>>>>> the program using llc and triggers the bug again.
>>>>>>>>>> 
>>>>>>>>>> I couldn't find other transforms that use the isOperation* callbacks, so
>>>>>>>>>> I think you may have to add a function to convert from IR Instruction to
>>>>>>>>>> SelectionDAG Opcodes or implement this some other way.
>>>>>>>>> I think a specific hook in TargetLowering may be the best approach here.
>>>>>>>>> Let me see what I can come up with.
>>>>>>>>> If you have ideas on the naming, let me know :).
>>>>>>>> In fact, we may be able to use this:
>>>>>>>> int TargetLoweringBase::InstructionOpcodeToISD(unsigned Opcode)const
>>>>>>> 
>>>>>>> Oh, I didn't see that.  That should work.
>>>>>> Almost there.
>>>>>> I am seeing a link error when creating the bugpoint executable.
>>>>>> 
>>>>>> llvm[2]: Linking Release executable bugpoint (without symbols)
>>>>>> Undefined symbols for architecture x86_64:
>>>>>> "llvm::TargetLoweringBase::InstructionOpcodeToISD(unsigned int) const", referenced from:
>>>>>>    (anonymous namespace)::AddressingModeMatcher::MatchOperationAddr(llvm::User*, unsigned int, unsigned int, bool*) in libLLVMScalarOpts.a(CodeGenPrepare.o)
>>>>>> ld: symbol(s) not found for architecture x86_64
>>>>>> 
>>>>>> The thing that I do not understand is why the linker is not find this function, whereas it does not complain about TargetLoweringBase::isLegalAddressingMode.
>>>>>> I may have missed something obvious, but I do not see it.
>>>>> 
>>>>> I'm not seeing this error on my machine.  What build system are you
>>>>> using?
>>>> Makefile on Mac OS.
>>>> 
>>>>> Did you try running make clean first?
>>>> I’ve tried but it does not seem to make the trick.
>>>> 
>>>> Could you try the attached patch?
>>>> 
>>> 
>>> I've tested this and it builds fine for me on Linux with autoconf
>>> and it also fixes the testcase.
>>> 
>>> -Tom
>>> 
>>>> Thanks,
>>>> -Quentin
>>>> 
>>>>> 
>>>>> -Tom
>>>>> 
>>>>>> 
>>>>>> Any idea?
>>>>>> 
>>>>>> Thanks,
>>>>>> -Quentin
>>>>>>> 
>>>>>>> -Tom
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Cheers,
>>>>>>>>> Quentin
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Tom
>>>>>>>>>> <update-cgp-test.case.diff>
>>>>>>>>> 
>>>>>>>>> _______________________________________________
>>>>>>>>> llvm-commits mailing list
>>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 