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

Tom Stellard tom at stellard.net
Mon Feb 24 06:42:01 PST 2014 

On Fri, Feb 21, 2014 at 05:13:10PM -0800, Quentin Colombet wrote:
> Hi Tom,
> 
> The fix finally landed in r201923.

Great, thanks for working on this.

-Tom

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

More information about the llvm-commits mailing list