r201439 - [CodeGenPrepare][AddressingModeMatcher] Give up on type promotion if the
Quentin Colombet
qcolombet at apple.com
Fri Feb 21 17:13:10 PST 2014
Hi Tom,
The fix finally landed in r201923.
Cheers,
-Quentin
On Feb 19, 2014, at 11:58 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>
> 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
>>>
>
>
> _______________________________________________
> 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/20140221/03534640/attachment.html>
More information about the llvm-commits
mailing list