[llvm-dev] [LLVMdev] TableGen Register Class not matching for MI in 3.6
Ryan Taylor via llvm-dev
llvm-dev at lists.llvm.org
Mon Aug 24 18:49:52 PDT 2015
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?
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/20150824/1c5460fe/attachment.html>
More information about the llvm-dev
mailing list