llvm dev - Aug 2015 - [LLVMdev] TableGen Register Class not matching for MI in 3.6

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/4abdfc29/attachment-0001.html>

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 <mailto:qcolombet at apple.com>> wrote:
> 
>> On Aug 25, 2015, at 11:05 AM, Ryan Taylor <ryta1203 at gmail.com
<mailto: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 <mailto:qcolombet at apple.com>> wrote:
>> 
>>> On Aug 25, 2015, at 10:29 AM, Ryan Taylor <ryta1203 at gmail.com
<mailto: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 <mailto:qcolombet at apple.com>> wrote:
>>> 
>>>> On Aug 25, 2015, at 10:05 AM, Ryan Taylor <ryta1203 at
gmail.com <mailto: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 <mailto: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 <mailto:qcolombet at apple.com>> wrote:
>>>> 
>>>>> On Aug 25, 2015, at 9:36 AM, Ryan Taylor <ryta1203 at
gmail.com <mailto: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 <mailto:qcolombet at apple.com>> wrote:
>>>>> 
>>>>>> On Aug 25, 2015, at 9:23 AM, Ryan Taylor <ryta1203
at gmail.com <mailto: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 <mailto:qcolombet at apple.com>> wrote:
>>>>>> Hi Ryan,
>>>>>> 
>>>>>>> On Aug 24, 2015, at 6:49 PM, Ryan Taylor
<ryta1203 at gmail.com <mailto: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 <mailto: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 <mailto: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 <mailto:qcolombet at apple.com>> wrote:
>>>>>>> 
>>>>>>>> On Aug 24, 2015, at 4:46 PM, Ryan Taylor
<ryta1203 at gmail.com <mailto: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 <mailto:qcolombet at apple.com>>
wrote:
>>>>>>>> 
>>>>>>>>> On Aug 24, 2015, at 1:30 PM, Ryan Taylor
<ryta1203 at gmail.com <mailto: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 <mailto:qcolombet at apple.com>>
wrote:
>>>>>>>>> 
>>>>>>>>>> On Aug 22, 2015, at 9:10 AM, Ryan
Taylor <ryta1203 at gmail.com <mailto: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 <mailto: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 <mailto:qcolombet at
apple.com>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> On Aug 19, 2015, at 9:42 AM, Ryan
Taylor <ryta1203 at gmail.com <mailto: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 <mailto: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 <mailto: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 <mailto:ryta1203 at
gmail.com>> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> ---------- Forwarded message
----------
>>>>>>>>>>>> From: Ryan Taylor <ryta1203
at gmail.com <mailto: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 <mailto:qcolombet at apple.com>>
>>>>>>>>>>>> Cc: "llvmdev at
cs.uiuc.edu <mailto:llvmdev at cs.uiuc.edu>" <llvmdev at
cs.uiuc.edu <mailto: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 <mailto:qcolombet at
apple.com>> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>>> On Jul 31, 2015, at 10:14
AM, Ryan Taylor <ryta1203 at gmail.com <mailto: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 <mailto: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 <mailto: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
<mailto: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
<mailto: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=
<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/30fe9883/attachment-0001.html>

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>