[llvm-dev] [LLVMdev] TableGen Register Class not matching for MI in 3.6
Ryan Taylor via llvm-dev
llvm-dev at lists.llvm.org
Tue Aug 25 12:59:25 PDT 2015
BB#0: derived from LLVM BB %entry
%vreg0<def> = MOV16Copy_IMM_REG <ga:@a+1>[TF=1]; GPRRegs:%vreg0
%vreg1<def> = COPY %vreg0; PTRRegs:%vreg1 GPRRegs:%vreg0
Send_iii %NULLR0, %vreg1<kill>, 1, 1, 1, 1, 0; PTRRegs:%vreg1
RetRA
This is what I get. This is what I'd like to get:
BB#0: derived from LLVM BB %entry
%vreg0<def> = MOV16Copy_IMM_REG <ga:@a+1>[TF=1]; PTRRegs:%vreg0
Send_iii %NULLR0, %vreg1<kill>, 1, 1, 1, 1, 0; PTRRegs:%vreg0
RetRA
On Tue, Aug 25, 2015 at 3:56 PM, Quentin Colombet <qcolombet at apple.com>
wrote:
> Oh, could you paste the MIs you get right after ISel (the whole def use
> chain of the interesting vregs)?
>
> Q.
>
> On Aug 25, 2015, at 12:00 PM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>
> AddRegisterOperand calls getVR and yes, I think an IMPLICIT_DEF is being
> generated.
>
> On Tue, Aug 25, 2015 at 2:40 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
>>
>> On Aug 25, 2015, at 11:05 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>>
>> I have not tried 3.5, it's a significant amount of work to port from one
>> version to the next though, I did not personally do the 3.4 to 3.6 porting.
>> I agree though, it was very strange that it suddenly just changed behavior.
>>
>> It looks like to me that InstrEmitter.cpp:getVR is the one assigning the
>> virtual register no?
>>
>>
>> No, IIRC getVR only create the virtual register for implicit defs. Which
>> is not your case, right?
>>
>>
>> Though this code in CreateVirtualRegisters:
>>
>> const TargetRegisterClass *RC =
>> TRI->getAllocatableClass(TII->getRegClass(II, i, TRI, *MF));
>>
>> That returns GPRBaseRegs for RC, but it then decides to constrain it
>> based on type:
>>
>> if (i < NumResults && TLI->isTypeLegal(Node->getSimpleValueType(i))) {
>> const TargetRegisterClass *VTRC =
>> TLI->getRegClassFor(Node->getSimpleValueType(i));
>> errs()<<"CVR VTRC: "<<VTRC->getID()<<"\n";
>> if (RC)
>> VTRC = TRI->getCommonSubClass(RC, VTRC);
>> if (VTRC)
>> RC = VTRC;
>> }
>>
>> VTRC = GPRRegs. Then RC=VTRC makes RC = GPRRegs.
>> the TLI info is from addRegisterClass(...) I'm assuming, which is defined
>> as GPRRegs in XXXISelLowering.cpp.
>>
>> It seems this is where it gets constrained even more. Honestly, I really
>> don't understand this part at all, why even have this type checking? If we
>> have defined a RegClass for that instruction, it should use that regclass
>> or subregclasses (depending on use/def info), correct?
>>
>>
>> I would believe so. Let me have a look to CreateVirtualRegisters to see
>> what I can tell you.
>>
>> Cheers,
>> -Quentin
>>
>>
>>
>>
>>
>> On Tue, Aug 25, 2015 at 1:37 PM, Quentin Colombet <qcolombet at apple.com>
>> wrote:
>>
>>>
>>> On Aug 25, 2015, at 10:29 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>>>
>>> 1. MOV16Copy_IMM_REG is the instruction matched, sorry. AD is the
>>> multiclass. The IMM in my case is a global. So you can see that
>>> GPRBaseRegs, GPRBaseRegs sets the registerclass for both the src and dst
>>> operands, in this case (MOV16Copy_IMM_REG) it's the dst.
>>>
>>> 2. Yes I agree, it most likely would.
>>>
>>> Honestly, this comes across like a bug, or unintended feature. It's
>>> adding an extra COPY to move from a GPR to a Base when a Base is perfectly
>>> allowed for the MOV16Copy instruction.
>>>
>>> I really just want to know if there is any way currently to get the TD
>>> defined register class for an operand for a machine instruction. There must
>>> be a way since LLVM produces valid registers for the operands.
>>>
>>>
>>> Definitely look into why InstrEmitter::CreateVirtualRegisters does not
>>> do what you want.
>>> The funny thing is that I do not remember we changed anything recently
>>> here, so this change of behavior is very strange to me.
>>> Have you tried 3.5 as well, maybe we could narrow down the commit range
>>> and understand what introduces the problem.
>>>
>>> Cheers,
>>> -Quentin
>>>
>>>
>>>
>>>
>>> On Tue, Aug 25, 2015 at 1:18 PM, Quentin Colombet <qcolombet at apple.com>
>>> wrote:
>>>
>>>>
>>>> On Aug 25, 2015, at 10:05 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>>>>
>>>> Here is the instruction in question:
>>>>
>>>> multiclass AD<string asmstr, SDPatternOperator OpNode, RegisterClass
>>>> srcAReg,
>>>> RegisterClass dstReg, ValueType srcAType,
>>>> ValueType dstType, Operand ImmOd, ImmLeaf imm_type>
>>>> {
>>>> def REG_REG : SetADInOut<asmstr, srcAReg, dstReg,
>>>> [(set dstReg:$dstD, (OpNode
>>>> srcAReg:$srcA))]>;
>>>> def IMM_REG : SetADInOut<asmstr, ImmOd, dstReg,
>>>> [(set dstReg:$dstD, (OpNode
>>>> imm_type:$srcA))]>;
>>>> def IMM_MEM : SetADIn<asmstr, ImmOd, memhx,
>>>> [(directStore (dstType (OpNode
>>>> imm_type:$srcA)), addr16:$dstD)]>;
>>>> def MEM_REG : SetADInOut<asmstr, memhx, dstReg,
>>>> [(set dstReg:$dstD, (OpNode (srcAType
>>>> (load addr16:$srcA))))]>;
>>>> def REG_MEM : SetADIn<asmstr, srcAReg, memhx,
>>>> [(directStore (dstType (OpNode
>>>> srcAReg:$srcA)), addr16:$dstD)]>;
>>>> def MEM_MEM : SetADIn<asmstr, memhx, memhx,
>>>> [(directStore (dstType (OpNode (srcAType
>>>> (load addr16:$srcA)))), addr16:$dstD)]>;
>>>> }
>>>>
>>>> defm MOV16Copy_ : AD<"mov16", null_frag, GPRBaseRegs,
>>>> GPRBaseRegs, i16, i16, simm16, immSExt16x>;
>>>>
>>>>
>>>> What is defining VReg?
>>>> It is AD or is it MOV16Copy?
>>>>
>>>> Also what are the arguments of the multiclass AD that you match?
>>>>
>>>>
>>>> On Tue, Aug 25, 2015 at 1:02 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>> wrote:
>>>>
>>>>> Quentin,
>>>>>
>>>>> 1. I'll take a look, it's also picking the reg class by the
>>>>> SimpleValueType and then getting the common subclass. Choosing to constrain
>>>>> the reg class to GPRRegs instead of GPRBaseRegs seems like it could lead to
>>>>> unintended spilling? If GPRBaseRegs was used then you could have the base
>>>>> reg class to choose from instead of spilling.
>>>>>
>>>>
>>>> Before spilling we try splitting, which relax the constraint on the
>>>> registers.
>>>> E.g.,
>>>> a(GPR) = def
>>>> = use a(GPR)
>>>>
>>>> After splitting:
>>>> a(GPR) = def
>>>> b(GPRBase) = COPY a(GPR)
>>>> c(GPR) = COPY b(GPRBase)
>>>> = use c(GPR)
>>>>
>>>> But, yes, if we could choose the most relaxed RC in the first place, we
>>>> would likely generate better code.
>>>>
>>>>
>>>> 2. Ok, yes, that makes sense.
>>>>> 3. I'll send a second email.
>>>>>
>>>>> Thanks.
>>>>>
>>>>> On Tue, Aug 25, 2015 at 12:49 PM, Quentin Colombet <
>>>>> qcolombet at apple.com> wrote:
>>>>>
>>>>>>
>>>>>> On Aug 25, 2015, at 9:36 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>>>>>>
>>>>>> Quentin,
>>>>>>
>>>>>> Yes, this is bound already:
>>>>>>
>>>>>> def GPRRegs: RegisterClass<"us", [i32], i32, (add R0,..... RX)>;
>>>>>> def BaseRegs: RegisterClass<"us", [i32], i32, (add B0...... BX)>;
>>>>>> def GPRBaseRegs: RegisterClass<"us", [i32], i32, (add BaseRegs,
>>>>>> GPRRegs)>;
>>>>>>
>>>>>> This is not the issue I think. It seems that it's first choosing the
>>>>>> subclass of GPRRegs for VReg but it then needs the dst in BaseRegs so it
>>>>>> creates a new virtual reg (NewVReg) with BaseRegsClass and generates a COPY
>>>>>> from GPRRegs to BaseRegsClass.
>>>>>>
>>>>>> 1. Why is it choosing the subclass for VReg instead of GPRBaseRegs?
>>>>>>
>>>>>>
>>>>>> To answer this question, you need to track down the creation of VReg.
>>>>>> I am guessing you want to look what is going on in
>>>>>> InstrEmitter::CreateVirtualRegisters.
>>>>>>
>>>>>>
>>>>>> 2. Why is it choosing the subclass for NewVReg instead of GPRBaseRegs?
>>>>>>
>>>>>>
>>>>>> I thought we had understood this point. NewVReg is used in an
>>>>>> instruction that uses a BaseRegs, so it cannot use GPRBaseRegs as it will
>>>>>> violate the constraint on that instruction.
>>>>>>
>>>>>>
>>>>>> The issue is that it's choosing the maximal subclass as the register
>>>>>> class for the mov instead of the td given superclass GPRBaseRegs... which
>>>>>> then makes it impossible to constrain it when comparing GPRRegs and
>>>>>> BaseRegs (they have no common subclass). What does have a common subclass
>>>>>> is GPRBaseRegs and BaseRegs, it's BaseRegs... so if the correct register
>>>>>> class (GPRBaseRegs) had been chosen for the MI in the first place, then
>>>>>> this seems like it could have been constrained instead of having to add the
>>>>>> useless COPY.
>>>>>>
>>>>>>
>>>>>> What are exactly the definitions (td) of the two instructions causing
>>>>>> the problem?
>>>>>>
>>>>>>
>>>>>> The instruction has GPRBaseRegs for both src and dst has it's RCs.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> On Tue, Aug 25, 2015 at 12:29 PM, Quentin Colombet <
>>>>>> qcolombet at apple.com> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Aug 25, 2015, at 9:23 AM, Ryan Taylor <ryta1203 at gmail.com> wrote:
>>>>>>>
>>>>>>> Quentin,
>>>>>>>
>>>>>>> 1. I'm looking at AddRegisterOperand in InstrEmitter.cpp
>>>>>>> 2. I'm not sure what you mean. In InstrInfo.td the MI is using
>>>>>>> GPRBase reg class for both src and dst (it's a mov MI). I need a class just
>>>>>>> for GPR also, since some operands can only map to GPR and not GPRBase, so I
>>>>>>> can't just replace GPR with GPRBase.
>>>>>>>
>>>>>>>
>>>>>>> You need to look at XXXRegisterInfo.td.
>>>>>>> You should have something like:
>>>>>>> def GPR : RegisterClass<"ARM", [i32], 32, (add (seque
>>>>>>>
>>>>>>> The list between square brackets are the type bound to this register
>>>>>>> class.
>>>>>>> You may want to bound i32 (or whatever) on GPRBase if that is not
>>>>>>> already the case.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> On Tue, Aug 25, 2015 at 12:18 PM, Quentin Colombet <
>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>
>>>>>>>> Hi Ryan,
>>>>>>>>
>>>>>>>> On Aug 24, 2015, at 6:49 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Quentin,
>>>>>>>>
>>>>>>>> I apologize for the spamming here but in getVR (where VReg is
>>>>>>>> assigned an RC), it calls:
>>>>>>>>
>>>>>>>> const TargetRegisterClass *RC =
>>>>>>>> TLI->getRegClassFor(Op.getSimpleValueType());
>>>>>>>> VReg = MRI->createVirtualRegister(RC);
>>>>>>>>
>>>>>>>> My question is why is it using the SimpleValueType to define the
>>>>>>>> register class instead of the actual register class defined in the td? What
>>>>>>>> am I missing here?
>>>>>>>>
>>>>>>>>
>>>>>>>> Right now, the types are bound to register classes. See
>>>>>>>> YourTargetRegisterInfo.td for the description of that mapping. I believe
>>>>>>>> that we first create a VReg using that RC then constraint it with the RC in
>>>>>>>> the td.
>>>>>>>> Two things:
>>>>>>>> 1. You can point me where you saw that and I can give you the exact
>>>>>>>> meaning of the snippet.
>>>>>>>> 2. You can change the mapping of your type in your RegisterInfo.td
>>>>>>>> to map GPRBase instead of GPR and see if it does what you want.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> -Quentin
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> On Mon, Aug 24, 2015 at 8:58 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Quentin,
>>>>>>>>>
>>>>>>>>> This is the issue. Somewhere prior to the constrainRegClass, it's
>>>>>>>>> assigning the GPRBase sub class of GPR to the MOV instruction, so it can't
>>>>>>>>> constrain it to Base and hence has to add the COPY. Now I just need to find
>>>>>>>>> out why it is ignoring the TableGen defined GPRBase for the MOV MI in favor
>>>>>>>>> of it's sub class GPR.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>> On Mon, Aug 24, 2015 at 8:34 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Quentin,
>>>>>>>>>>
>>>>>>>>>> It looks like firstCommonClass is returning a nullptr, which is
>>>>>>>>>> odd since the MOV should be using GPRBase (GPR and Base) and the NewVReg
>>>>>>>>>> class is Base. Maybe the ISel has decided to select the sub class GPR from
>>>>>>>>>> GPRBase and hence GPR != Base and so the constrainRegClass is failing.
>>>>>>>>>>
>>>>>>>>>> Using the MI Op's reg class and comparing it directly to the
>>>>>>>>>> NewVReg class would eliminate this possible issue and should produce more
>>>>>>>>>> accurate results?
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> On Mon, Aug 24, 2015 at 8:08 PM, Quentin Colombet <
>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Aug 24, 2015, at 4:46 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Here is the snippet that matters:
>>>>>>>>>>>
>>>>>>>>>>> void
>>>>>>>>>>> InstrEmitter::AddRegisterOperand(MachineInstrBuilder &MIB,
>>>>>>>>>>> SDValue Op,
>>>>>>>>>>> unsigned IIOpNum,
>>>>>>>>>>> const MCInstrDesc *II,
>>>>>>>>>>> DenseMap<SDValue, unsigned>
>>>>>>>>>>> &VRBaseMap,
>>>>>>>>>>> bool IsDebug, bool IsClone,
>>>>>>>>>>> bool IsCloned) {
>>>>>>>>>>> //llvm::errs() << "Op = ";
>>>>>>>>>>> //Op.dump();
>>>>>>>>>>> assert(Op.getValueType() != MVT::Other &&
>>>>>>>>>>> Op.getValueType() != MVT::Glue &&
>>>>>>>>>>> "Chain and glue operands should occur at end of operand
>>>>>>>>>>> list!");
>>>>>>>>>>> // Get/emit the operand.
>>>>>>>>>>> unsigned VReg = getVR(Op, VRBaseMap);
>>>>>>>>>>> assert(TargetRegisterInfo::isVirtualRegister(VReg) && "Not a
>>>>>>>>>>> vreg?");
>>>>>>>>>>> const MCInstrDesc &MCID = MIB->getDesc();
>>>>>>>>>>> bool isOptDef = IIOpNum < MCID.getNumOperands() &&
>>>>>>>>>>> MCID.OpInfo[IIOpNum].isOptionalDef();
>>>>>>>>>>> // If the instruction requires a register in a different
>>>>>>>>>>> class, create
>>>>>>>>>>> // a new virtual register and copy the value into it, but
>>>>>>>>>>> first attempt to
>>>>>>>>>>> // shrink VReg's register class within reason. For example,
>>>>>>>>>>> if VReg == GR32
>>>>>>>>>>> // and II requires a GR32_NOSP, just constrain VReg to
>>>>>>>>>>> GR32_NOSP.
>>>>>>>>>>> if (II) {
>>>>>>>>>>> const TargetRegisterClass *DstRC = nullptr;
>>>>>>>>>>> if (IIOpNum < II->getNumOperands())
>>>>>>>>>>> DstRC =
>>>>>>>>>>> TRI->getAllocatableClass(TII->getRegClass(*II,IIOpNum,TRI,*MF));
>>>>>>>>>>> if (DstRC && !MRI->constrainRegClass(VReg, DstRC,
>>>>>>>>>>> MinRCSize)) {
>>>>>>>>>>> unsigned NewVReg = MRI->createVirtualRegister(DstRC);
>>>>>>>>>>> if (TRI->getCommonSubClass(DstRC,
>>>>>>>>>>> TRI->getRegClass(II->OpInfo[IIOpNum].RegClass))
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> What I was saying was this II->OpInfo[IIOpNum].RegClass looks to
>>>>>>>>>>> return DstRC at first glance.
>>>>>>>>>>> What you want is TRI->getRegClass(VReg).
>>>>>>>>>>>
>>>>>>>>>>> BTW, now with the full snippet, I see your mistake. You are
>>>>>>>>>>> passing a RegClass to a method that expect a virtual register (your call is
>>>>>>>>>>> to TargetRegisterInfo::getRegClass I believe, not
>>>>>>>>>>> MCRegisterInfo::getRegClass). So you end up with the regclass of a virtual
>>>>>>>>>>> register numbered RegClass.
>>>>>>>>>>>
>>>>>>>>>>> Anyway, if we cannot constrain VReg on DstRC (i.e., what we try
>>>>>>>>>>> in the previous if), this means that getCommonSubClass will fail or will
>>>>>>>>>>> return a class that is likely too small to be reasonable (i.e., below
>>>>>>>>>>> MinRCSize).
>>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> -Quentin
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> == DstRC) {
>>>>>>>>>>> MRI->setRegClass(VReg, DstRC);
>>>>>>>>>>> }
>>>>>>>>>>> else {
>>>>>>>>>>> BuildMI(*MBB, InsertPos, Op.getNode()->getDebugLoc(),
>>>>>>>>>>> TII->get(TargetOpcode::COPY),
>>>>>>>>>>> NewVReg).addReg(VReg);
>>>>>>>>>>> VReg = NewVReg;
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> This does not work. The logic seems sound though, you are
>>>>>>>>>>> checking an RC (DstRC) and the MI's operand's RegClass, get the common sub,
>>>>>>>>>>> which should either be or not be DstRC, right?
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Aug 24, 2015 at 4:44 PM, Quentin Colombet <
>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Aug 24, 2015, at 1:30 PM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I'm trying to do something like this:
>>>>>>>>>>>>
>>>>>>>>>>>> // Dst = NewVReg's reg class
>>>>>>>>>>>> // *II = MCInstrDesc
>>>>>>>>>>>> // IIOpNum = II Operand Num
>>>>>>>>>>>>
>>>>>>>>>>>> if (TRI->getCommonSubClass(DstRC,
>>>>>>>>>>>> TRI->getRegClass(II->OpInfo[IIOpNum].RegClass)) == DstRC)
>>>>>>>>>>>> MRI->setRegClass(VReg, DstRC);
>>>>>>>>>>>> else
>>>>>>>>>>>> BuildMI(... TargetOpcode::COPY...)
>>>>>>>>>>>>
>>>>>>>>>>>> The condition is trying to reset the reg class if the DstRC reg
>>>>>>>>>>>> class is valid for the operand num of the machine instruction. If the
>>>>>>>>>>>> NewVReg register class is not valid for that operand of the machine
>>>>>>>>>>>> instruction I want to generate a COPY instruction (as it does now all the
>>>>>>>>>>>> time). This condition should check for intersection, right?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, that’s right.
>>>>>>>>>>>>
>>>>>>>>>>>> It should find the common subclass of the new register class
>>>>>>>>>>>> and the valid reg class of the operand for that machine instruction.
>>>>>>>>>>>>
>>>>>>>>>>>> Unfortunately it looks like that condition is always being set
>>>>>>>>>>>> to true (or I'm getting a TON of false positives).
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Where does II come from?
>>>>>>>>>>>> From the snippet, I am guessing it is the instruction that uses
>>>>>>>>>>>> NewVReg, i.e., you are checking that the class for NewVReg matches the
>>>>>>>>>>>> class for NewVReg… which by construction is always true!
>>>>>>>>>>>>
>>>>>>>>>>>> You want to check "common subclass” of DstRC and SrcRC.
>>>>>>>>>>>>
>>>>>>>>>>>> Cheers,
>>>>>>>>>>>> Q.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, Aug 24, 2015 at 2:09 PM, Quentin Colombet <
>>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Aug 22, 2015, at 9:10 AM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> One last question regarding this please.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Why aren't we simply changing the register class in
>>>>>>>>>>>>> AddRegisterOperand instead of building a new COPY? I admit I haven't
>>>>>>>>>>>>> thought this out but for my test cases so far this works just fine and
>>>>>>>>>>>>> reduces the number of ASM mov instructions that are being produced.
>>>>>>>>>>>>>
>>>>>>>>>>>>> For example, instead of BuildMI(...,
>>>>>>>>>>>>> TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg), use something like
>>>>>>>>>>>>> MRI->setRegClass(VReg, MRI->getRegClass(NewVReg)) ?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> The problem is that the old register class and the new one may
>>>>>>>>>>>>> not intersect. I do not know exactly what makes us create a new vreg
>>>>>>>>>>>>> w.r.t., but probably if the class is not identical we create one. You can
>>>>>>>>>>>>> try to use contraintsRegClass to get the intersection.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Q.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> What is the reasoning behind adding the extra instruction
>>>>>>>>>>>>> instead of changing the reg class?
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Aug 19, 2015 at 2:04 PM, Ryan Taylor <
>>>>>>>>>>>>> ryta1203 at gmail.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes, you're probably right about the ID. The odd part is that
>>>>>>>>>>>>>> I have other simpler instructions that use the same type of superset and it
>>>>>>>>>>>>>> always, so far, matches correctly (it doesn't just pick GPRRegs all the
>>>>>>>>>>>>>> time).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Like I said, we can just 'fill in the gaps' with new MIs but
>>>>>>>>>>>>>> that sure seems like a brush off solution. The td files would be so much
>>>>>>>>>>>>>> cleaner if you could have a superset reg class that actually matched the
>>>>>>>>>>>>>> correct reg usage (which it sort of does in AddRegisterOperand when it adds
>>>>>>>>>>>>>> the extra COPY.... not sure why it does this instead of just checking the
>>>>>>>>>>>>>> original MI and seeing if the reg class needed is in the list and then just
>>>>>>>>>>>>>> changing the vreg reg class for the original MI, that seems like a better
>>>>>>>>>>>>>> solution?) It's like it's actually picking some reg class first and then
>>>>>>>>>>>>>> trying to fix it's error by adding MORE instructions instead of finding the
>>>>>>>>>>>>>> right reg class the first time.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Wed, Aug 19, 2015 at 1:32 PM, Quentin Colombet <
>>>>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Aug 19, 2015, at 9:42 AM, Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It seems the problem arises from using multiple reg classes
>>>>>>>>>>>>>>> for one MI in the td file, I guess.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Probably, that does not sound something used widely :).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I'm not sure it takes first available, if I swap the reg
>>>>>>>>>>>>>>> classes in the list it does not change and if I replace the GPR reg class
>>>>>>>>>>>>>>> with something different than it picks the base reg class fine, potentially
>>>>>>>>>>>>>>> it is using the reg class with most available? idk.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> My guess is it would take the register class that come first
>>>>>>>>>>>>>>> in the register class IDs (not the list on the instruction itself), but I
>>>>>>>>>>>>>>> am just guessing.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I just need to create MIs for every possible case I guess.
>>>>>>>>>>>>>>> Thanks for the help! :)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Wed, Aug 19, 2015 at 12:04 PM, Quentin Colombet <
>>>>>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi Ryan,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Aug 19, 2015, at 6:35 AM, Ryan Taylor via llvm-dev <
>>>>>>>>>>>>>>>> llvm-dev at lists.llvm.org> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Essentially it doesn't appear that the reg class assignment
>>>>>>>>>>>>>>>> is based on uses and is instead inserting an extra COPY for this. Is this
>>>>>>>>>>>>>>>> accurate? If so, why?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> We match the instructions bottom-up and I believe that copy
>>>>>>>>>>>>>>>> are automatically inserted when the register classes do not match.
>>>>>>>>>>>>>>>> That seems strange to me that the isel logs are exactly the
>>>>>>>>>>>>>>>> same but still you are seeing a different result of instruction selection.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In this above example, I'm getting an extra "mov %r0, $b1"
>>>>>>>>>>>>>>>> (this is an MI::COPY) even though "mov @a, %b1" (this is an MI::MOV) is
>>>>>>>>>>>>>>>> entirely acceptable since both GPRRegs and BaseRegs are in the reg class
>>>>>>>>>>>>>>>> list..
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If the heuristic is simply picking the first available reg
>>>>>>>>>>>>>>>> class or the reg class with the most available, for example, instead of
>>>>>>>>>>>>>>>> picking the reg class based on uses, I will have to change this heuristic
>>>>>>>>>>>>>>>> to reduce code bloat, or we'll have to add backend passes to reduce the
>>>>>>>>>>>>>>>> code bloat.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I think the current approach is rather simple and we just
>>>>>>>>>>>>>>>> match the type for the first available reg class (assuming your selection
>>>>>>>>>>>>>>>> dag patterns are not choosing something different). There are not per say
>>>>>>>>>>>>>>>> passes to reduce this "code bloat” since we never encounter this problem in
>>>>>>>>>>>>>>>> practice. The peephole optimizer has some logic to avoid this move and
>>>>>>>>>>>>>>>> CodeGenPrepare also does a few transformation to avoid some of that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Anyhow, I’d say debug the isel process (probably the
>>>>>>>>>>>>>>>> InstrEmitter part) to see where the problem arise.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>> -Quentin
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Mon, Aug 17, 2015 at 7:00 PM, Ryan Taylor <
>>>>>>>>>>>>>>>> ryta1203 at gmail.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ---------- Forwarded message ----------
>>>>>>>>>>>>>>>>> From: Ryan Taylor <ryta1203 at gmail.com>
>>>>>>>>>>>>>>>>> Date: Mon, Aug 17, 2015 at 6:59 PM
>>>>>>>>>>>>>>>>> Subject: Re: [LLVMdev] TableGen Register Class not
>>>>>>>>>>>>>>>>> matching for MI in 3.6
>>>>>>>>>>>>>>>>> To: Quentin Colombet <qcolombet at apple.com>
>>>>>>>>>>>>>>>>> Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The isel logs are exactly the same, nothing changed, it's
>>>>>>>>>>>>>>>>> matching the same instructions just the reg classes are different.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Where in the SelectionDAG is the code that adds an extra
>>>>>>>>>>>>>>>>> MI COPY? I'll have to set a breakpoint and debug backwards from there to
>>>>>>>>>>>>>>>>> see what's going on. It's only happening with the GPRRegs class, for
>>>>>>>>>>>>>>>>> example if I use another reg class instead along with the base regs than it
>>>>>>>>>>>>>>>>> matches the base reg just fine, it's like GPR is overriding any other reg
>>>>>>>>>>>>>>>>> class matching.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Fri, Jul 31, 2015 at 1:21 PM, Quentin Colombet <
>>>>>>>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Jul 31, 2015, at 10:14 AM, Ryan Taylor <
>>>>>>>>>>>>>>>>>> ryta1203 at gmail.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Quentin,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> It's in the instruction selection, sorry I forgot to
>>>>>>>>>>>>>>>>>> mention that. The Vreg class is GPR and an extra COPY is generated to copy
>>>>>>>>>>>>>>>>>> from the GPR to the Base Reg, even though my 'mov' instruction has Base in
>>>>>>>>>>>>>>>>>> the Register class list.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Then, I suggest you use -debug-only=isel to check what
>>>>>>>>>>>>>>>>>> changed in your selection instruction process. I do not see off-hand what
>>>>>>>>>>>>>>>>>> could have caused.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Sorry for not being more helpful here.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Q.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Fri, Jul 31, 2015 at 12:50 PM, Quentin Colombet <
>>>>>>>>>>>>>>>>>> qcolombet at apple.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi Ryan,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Could you check where those moves come from?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> In particular, is this the product of the instruction
>>>>>>>>>>>>>>>>>>> selection process?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> You use -print-machineinstrs to see when it is inserted.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> -Quentin
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> > On Jul 30, 2015, at 2:02 PM, Ryan Taylor <
>>>>>>>>>>>>>>>>>>> ryta1203 at gmail.com> wrote:
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > In LLVM 3.6,
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > We have an instruction that uses a register class
>>>>>>>>>>>>>>>>>>> that is defined of several different reg classes. In 3.4 this works fine
>>>>>>>>>>>>>>>>>>> but in 3.6 this is broken.
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > For example, I have a mov instruction. mov can be
>>>>>>>>>>>>>>>>>>> executed between different register types (ie gpr, index, base, etc..)
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > In 3.4, we would get something like this:
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > mov @a, %b1 // moving this immediate to a base
>>>>>>>>>>>>>>>>>>> register, which is what we want
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > In 3.6, we now get this:
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > mov @a, %r0 // r0 = gpr
>>>>>>>>>>>>>>>>>>> > mov %r0, %b1 // b1 = base reg
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > The register class looks like this:
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > def ARegs : RegisterClass<"us", [i16], i16, (add
>>>>>>>>>>>>>>>>>>> GPRRegs, IndexRegs, BaseRegs)>;
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > I have absolutely no idea why this is not matching any
>>>>>>>>>>>>>>>>>>> longer?
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > The fix here is to define an MI with explicit single
>>>>>>>>>>>>>>>>>>> register class (ie it only allows PTRRegs as the destination).
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > This must be an issue with something else and not the
>>>>>>>>>>>>>>>>>>> tablegen but if that was the case I'm not sure. Anyway help would be great,
>>>>>>>>>>>>>>>>>>> what should I be looking at here?
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > Thanks.
>>>>>>>>>>>>>>>>>>> > _______________________________________________
>>>>>>>>>>>>>>>>>>> > LLVM Developers mailing list
>>>>>>>>>>>>>>>>>>> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>>>>>>>>>>>>>>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.cs.uiuc.edu_&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=7VQTHNyB_IcF4I86741RzVduPmdgCRL_mHUfuUmnL0Y&s=fK8KXlXNFjX9EyldRbiY4GwaxZBFjvTYEXwRJuFLJvg&e=>
>>>>>>>>>>>>>>>>>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>>>>>>>>>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.cs.uiuc.edu_mailman_listinfo_llvmdev&d=BQMFaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=7VQTHNyB_IcF4I86741RzVduPmdgCRL_mHUfuUmnL0Y&s=TAPJBdWJfY-53g2FEN-kOKTo2nDJxcpEGNpqpgcrN9U&e=>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>>>>>>>> llvm-dev at lists.llvm.org
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=Kar_gr4lUTX9iRgFLHyIPCR7VD3VlB3-02h_Q5v9oWk&m=7VQTHNyB_IcF4I86741RzVduPmdgCRL_mHUfuUmnL0Y&s=N0TOaZZSrDVRpbh_uMG5DV5ZZ2_KVMcBWtK9vGIaByY&e=
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150825/33a6aa7f/attachment.html>
More information about the llvm-dev
mailing list