<div dir="ltr"><div>Quentin,</div><div><br></div><div> The problem with making one addRegisterOperand per type is that you can have multiple registers per type and then you have this issue. It seems that fix is very architecture specific, in fact, I'm fairly surprised that my issue hasn't occurred with other architectures (though if you are running 'real' algorithms, a few extra movs here and there aren't really an issue on more traditional archs, so I'm not sure anyone would have even noticed... we do not have a traditional-type architecture). In fact, in my opinion, this simply doesn't look like a good solution for that SPARC issue, I'm guessing they weren't able to handle it via the target specific hooks/td files? Seems to me they could have chosen to expand the SparcInstrInfo.td and solved the issue, but that is just a first guess. </div><div><br></div><div>If I use GPRBase instead of just GPRRegs for addRegisterOperand(i16) then I'm going to have all sorts of other issues. We are basically using the addRegisterOperand as a 'default/base' reg class for the type so GPR is good for that, but we also want to be able to use specific registers in certain instances that are also i16. I think a better solution for us is to undo these changes in the core code, we'll just have to make the adjustments every time we port LLVM. Again this is just my opinion but LLVM seems somewhat lacking robustness/flexibility in this area, though I realize it's target use isn't novel architectures, nor was it constructed with them in mind.</div><div><br></div><div> Well, I appreciate your time and patience on this one! You have really helped me get to the bottom of it. I'm going to have to revert those few lines of code in CreateVirtualRegisters in the InstrEmitter.cpp.</div><div><br></div><div>Thanks again Quentin!</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 5:38 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="-ms-word-wrap: break-word;">There is not an implicit def here :).<div><br></div><div>Anyhow, the change in behavior happened last year with r199187:</div><div><div style="margin:0px;font-family:Menlo;font-size:11px">Author: Jakob Stoklund Olesen <<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>></div><div style="margin:0px;font-family:Menlo;font-size:11px">Date: Tue Jan 14 06:18:38 2014 +0000</div><div style="margin:0px;font-family:Menlo;font-size:11px;min-height:13px"><br></div><div style="margin:0px;font-family:Menlo;font-size:11px"> Always let value types influence register classes.</div><p style="margin:0px;font-family:Menlo;font-size:11px;min-height:13px"> <br></p><div style="margin:0px;font-family:Menlo;font-size:11px"> When creating a virtual register for a def, the value type should be</div><div style="margin:0px;font-family:Menlo;font-size:11px"> used to pick the register class. If we only use the register class</div><div style="margin:0px;font-family:Menlo;font-size:11px"> constraint on the instruction, we might pick a too large register class.</div><p style="margin:0px;font-family:Menlo;font-size:11px;min-height:13px"> <br></p><div style="margin:0px;font-family:Menlo;font-size:11px"> Some registers can store values of different sizes. For example, the x86</div><div style="margin:0px;font-family:Menlo;font-size:11px"> xmm registers can hold f32, f64, and 128-bit vectors. The three</div><div style="margin:0px;font-family:Menlo;font-size:11px"> different value sizes are represented by register classes with identical</div><div style="margin:0px;font-family:Menlo;font-size:11px"> register sets: FR32, FR64, and VR128. These register classes have</div><div style="margin:0px;font-family:Menlo;font-size:11px"> different spill slot sizes, so it is important to use the right one.</div><p style="margin:0px;font-family:Menlo;font-size:11px;min-height:13px"> <br></p><div style="margin:0px;font-family:Menlo;font-size:11px"> The register class constraint on an instruction doesn't necessarily care</div><div style="margin:0px;font-family:Menlo;font-size:11px"> about the size of the value its defining. The value type determines</div><div style="margin:0px;font-family:Menlo;font-size:11px"> that.</div><p style="margin:0px;font-family:Menlo;font-size:11px;min-height:13px"> <br></p><div style="margin:0px;font-family:Menlo;font-size:11px"> This fixes a problem where InstrEmitter was picking 32-bit register</div><div style="margin:0px;font-family:Menlo;font-size:11px"> classes for 64-bit values on SPARC.</div><div><br></div><div>So if you want to fix that on your end, I guess you would need to specify that i16 is best placed on GPRBaseRegs instead of GPRRegs, via the TLI hooks.</div><div><br></div><div><div><div class="h5"><blockquote type="cite"><div>On Aug 25, 2015, at 12:59 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>BB#0: derived from LLVM BB %entry<br> %vreg0<def> = MOV16Copy_IMM_REG <ga:@a+1>[TF=1]; GPRRegs:%vreg0<br> %vreg1<def> = COPY %vreg0; PTRRegs:%vreg1 GPRRegs:%vreg0<br> Send_iii %NULLR0, %vreg1<kill>, 1, 1, 1, 1, 0; PTRRegs:%vreg1<br> RetRA</div><div><br></div><div>This is what I get. This is what I'd like to get:</div><div><br></div><div>BB#0: derived from LLVM BB %entry<br> %vreg0<def> = MOV16Copy_IMM_REG <ga:@a+1>[TF=1]; PTRRegs:%vreg0<br> Send_iii %NULLR0, %vreg1<kill>, 1, 1, 1, 1, 0; PTRRegs:%vreg0<br> RetRA<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 3:56 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div>Oh, could you paste the MIs you get right after ISel (the whole def use chain of the interesting vregs)?<span><font color="#888888"><div><br></div></font></span><div><span><font color="#888888">Q.</font></span><div><div><br><div><blockquote type="cite"><div>On Aug 25, 2015, at 12:00 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr">AddRegisterOperand calls getVR and yes, I think an IMPLICIT_DEF is being generated.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 2:40 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 25, 2015, at 11:05 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>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.</div><div><br></div><div>It looks like to me that InstrEmitter.cpp:getVR is the one assigning the virtual register no?</div></div></div></blockquote><div><br></div></span><div>No, IIRC getVR only create the virtual register for implicit defs. Which is not your case, right?</div><span><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Though this code in CreateVirtualRegisters:</div><div><br></div><div>const TargetRegisterClass *RC =<br> TRI->getAllocatableClass(TII->getRegClass(II, i, TRI, *MF));</div><div><br></div><div>That returns GPRBaseRegs for RC, but it then decides to constrain it based on type:</div><div><br></div><div> if (i < NumResults && TLI->isTypeLegal(Node->getSimpleValueType(i))) {<br> const TargetRegisterClass *VTRC =<br> TLI->getRegClassFor(Node->getSimpleValueType(i));<br> errs()<<"CVR VTRC: "<<VTRC->getID()<<"\n";<br> if (RC)<br> VTRC = TRI->getCommonSubClass(RC, VTRC);<br> if (VTRC)<br> RC = VTRC;<br> }<br></div><div><br></div><div>VTRC = GPRRegs. Then RC=VTRC makes RC = GPRRegs.</div><div>the TLI info is from addRegisterClass(...) I'm assuming, which is defined as GPRRegs in XXXISelLowering.cpp.</div></div></div></blockquote></span></div></div></blockquote></div></div></div></blockquote></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></div></div><div>I believe you are right.</div><span><br><blockquote type="cite"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><div><div><div><div><blockquote type="cite"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><div><span><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>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?</div></div></div></blockquote></span></div></div></blockquote></div></div></div></blockquote></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>See the comment from Jakob on the commit. But basically, this helps the compiler taking the right size for the spill slots.</div><div><div class="h5"><br><blockquote type="cite"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><div><div><div><div><blockquote type="cite"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><div><span><blockquote type="cite"><div><div dir="ltr"><div>If we have defined a RegClass for that instruction, it should use that regclass or subregclasses (depending on use/def info), correct?<br></div></div></div></blockquote><div><br></div></span><div>I would believe so. Let me have a look to CreateVirtualRegisters to see what I can tell you.</div><div><div><div><br></div><div>Cheers,</div><div>-Quentin</div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 1:37 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 25, 2015, at 10:29 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>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.</div><div><br></div><div>2. Yes I agree, it most likely would. </div><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div></blockquote><div><br></div></span><div>Definitely look into why <span style="font-family:Menlo;font-size:11px">InstrEmitter::CreateVirtualRegisters </span>does not do what you want.</div><div>The funny thing is that I do not remember we changed anything recently here, so this change of behavior is very strange to me.</div><div>Have you tried 3.5 as well, maybe we could narrow down the commit range and understand what introduces the problem.</div><div><br></div><div>Cheers,</div><div>-Quentin</div><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 1:18 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 25, 2015, at 10:05 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>Here is the instruction in question:</div><div><br></div><div>multiclass AD<string asmstr, SDPatternOperator OpNode, RegisterClass srcAReg,<br> RegisterClass dstReg, ValueType srcAType,<br> ValueType dstType, Operand ImmOd, ImmLeaf imm_type><br>{<br> def REG_REG : SetADInOut<asmstr, srcAReg, dstReg,<br> [(set dstReg:$dstD, (OpNode srcAReg:$srcA))]>;<br> def IMM_REG : SetADInOut<asmstr, ImmOd, dstReg,<br> [(set dstReg:$dstD, (OpNode imm_type:$srcA))]>;<br> def IMM_MEM : SetADIn<asmstr, ImmOd, memhx,<br> [(directStore (dstType (OpNode imm_type:$srcA)), addr16:$dstD)]>;<br> def MEM_REG : SetADInOut<asmstr, memhx, dstReg,<br> [(set dstReg:$dstD, (OpNode (srcAType (load addr16:$srcA))))]>;<br> def REG_MEM : SetADIn<asmstr, srcAReg, memhx,<br> [(directStore (dstType (OpNode srcAReg:$srcA)), addr16:$dstD)]>;<br> def MEM_MEM : SetADIn<asmstr, memhx, memhx,<br> [(directStore (dstType (OpNode (srcAType (load addr16:$srcA)))), addr16:$dstD)]>;<br>}</div><p>defm MOV16Copy_ : AD<"mov16", null_frag, GPRBaseRegs, GPRBaseRegs, i16, i16, simm16, immSExt16x>;<br></p></div></div></blockquote><div><br></div></span><div>What is defining VReg?</div><div>It is AD or is it MOV16Copy?</div><div><br></div><div>Also what are the arguments of the multiclass AD that you match?</div><span><br><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 1:02 PM, Ryan Taylor <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><div>Quentin,</div><div><br></div><div> 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.</div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>Before spilling we try splitting, which relax the constraint on the registers.</div><div>E.g.,</div><div>a(GPR) = def</div><div>= use a(GPR)</div><div><br></div><div>After splitting:</div><div><div>a(GPR) = def</div><div>b(GPRBase) = COPY a(GPR)</div><div>c(GPR) = COPY b(GPRBase)</div><div>= use c(GPR)</div><div><br></div><div>But, yes, if we could choose the most relaxed RC in the first place, we would likely generate better code.</div><div><br></div></div><div><div><br><blockquote type="cite"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><div> 2. Ok, yes, that makes sense.</div><div> 3. I'll send a second email.</div><div><br></div><div>Thanks.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 12:49 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><blockquote type="cite"><span><div>On Aug 25, 2015, at 9:36 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br></span><div><div dir="ltr"><div>Quentin,</div><div><br></div><span><div> Yes, this is bound already:</div><div><br></div><div>def GPRRegs: RegisterClass<"us", [i32], i32, (add R0,..... RX)>;</div><div>def BaseRegs: RegisterClass<"us", [i32], i32, (add B0...... BX)>;</div><div>def GPRBaseRegs: RegisterClass<"us", [i32], i32, (add BaseRegs, GPRRegs)>;</div><div><br></div><div>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.</div><div><br></div><div>1. Why is it choosing the subclass for VReg instead of GPRBaseRegs?</div></span></div></div></blockquote><div><br></div><div>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 <span style="font-family:Menlo;font-size:11px">InstrEmitter::CreateVirtualRegisters</span>.</div><span><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div>2. Why is it choosing the subclass for NewVReg instead of GPRBaseRegs?</div></div></div></blockquote><div><br></div></span><div>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.</div><span><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>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.</div></div></div></blockquote><div><br></div></span><div>What are exactly the definitions (td) of the two instructions causing the problem? </div><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>The instruction has GPRBaseRegs for both src and dst has it's RCs.</div><div><br></div><div>Thanks.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 12:29 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><blockquote type="cite"><span><div>On Aug 25, 2015, at 9:23 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br></span><div><div dir="ltr"><div>Quentin,</div><div><br></div><span><div> 1. I'm looking at AddRegisterOperand in InstrEmitter.cpp</div><div> 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. </div></span></div></div></blockquote><div><br></div><div>You need to look at XXXRegisterInfo.td.</div><div>You should have something like:</div><div><div style="margin:0px;font-family:Menlo;font-size:11px">def GPR : RegisterClass<"ARM", [i32], 32, (add (seque</div></div><div><br></div><div>The list between square brackets are the type bound to this register class.</div><div>You may want to bound i32 (or whatever) on GPRBase if that is not already the case.</div><div><div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Thanks.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 25, 2015 at 12:18 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div>Hi Ryan,<div><br><div><blockquote type="cite"><span><div>On Aug 24, 2015, at 6:49 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br></span><div><div dir="ltr"><div>Quentin,</div><div><br></div><span><div> I apologize for the spamming here but in getVR (where VReg is assigned an RC), it calls:</div><div><br></div><div>const TargetRegisterClass *RC = TLI->getRegClassFor(Op.getSimpleValueType());</div><div>VReg = MRI->createVirtualRegister(RC);</div><div><br></div><div>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?</div></span></div></div></blockquote><div><br></div><div>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.</div><div>Two things:</div><div>1. You can point me where you saw that and I can give you the exact meaning of the snippet.</div><div>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.</div><div><br></div><div>Cheers,</div><div>-Quentin</div><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Thanks!</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 8:58 PM, Ryan Taylor <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><div>Quentin,</div><div><br></div><div> 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.</div><div><br></div><div>Thanks.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 8:34 PM, Ryan Taylor <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><div>Quentin,</div><div><br></div><div> 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. </div><div><br></div><div> 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?</div><div><br></div><div>Thanks.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 8:08 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><div><div><blockquote type="cite"><div>On Aug 24, 2015, at 4:46 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>Here is the snippet that matters:</div><div><br></div><div>void<br>InstrEmitter::AddRegisterOperand(MachineInstrBuilder &MIB,<br> SDValue Op,<br> unsigned IIOpNum,<br> const MCInstrDesc *II,<br> DenseMap<SDValue, unsigned> &VRBaseMap,<br> bool IsDebug, bool IsClone, bool IsCloned) {<br> //llvm::errs() << "Op = ";<br> //Op.dump();<br> assert(Op.getValueType() != MVT::Other &&<br> Op.getValueType() != MVT::Glue &&<br> "Chain and glue operands should occur at end of operand list!");<br> // Get/emit the operand.<br> unsigned VReg = getVR(Op, VRBaseMap);<br> assert(TargetRegisterInfo::isVirtualRegister(VReg) && "Not a vreg?");</div><div> const MCInstrDesc &MCID = MIB->getDesc();<br> bool isOptDef = IIOpNum < MCID.getNumOperands() &&<br> MCID.OpInfo[IIOpNum].isOptionalDef();</div><div> // If the instruction requires a register in a different class, create<br> // a new virtual register and copy the value into it, but first attempt to<br> // shrink VReg's register class within reason. For example, if VReg == GR32<br> // and II requires a GR32_NOSP, just constrain VReg to GR32_NOSP.<br> if (II) {<br> const TargetRegisterClass *DstRC = nullptr;<br> if (IIOpNum < II->getNumOperands())<br> DstRC = TRI->getAllocatableClass(TII->getRegClass(*II,IIOpNum,TRI,*MF));<br> if (DstRC && !MRI->constrainRegClass(VReg, DstRC, MinRCSize)) {<br> unsigned NewVReg = MRI->createVirtualRegister(DstRC);<br> if (TRI->getCommonSubClass(DstRC,<br> TRI->getRegClass(II->OpInfo[IIOpNum].RegClass))<br></div></div></div></blockquote><div><br></div></div></div><div>What I was saying was this II->OpInfo[IIOpNum].RegClass looks to return DstRC at first glance.</div><div>What you want is TRI->getRegClass(VReg).</div><div><br></div><div>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.</div><div><br></div><div>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).</div><div><br></div><div>Cheers,</div><div>-Quentin</div><div><div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div> == DstRC) {<br> MRI->setRegClass(VReg, DstRC);<br> }<br> else {<br> BuildMI(*MBB, InsertPos, Op.getNode()->getDebugLoc(),<br> TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg);<br> VReg = NewVReg;<br> }<br> }</div><div> }</div><div><br></div><div>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?</div><div><br></div><div>Thanks.<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 4:44 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 24, 2015, at 1:30 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>I'm trying to do something like this:</div><div><br></div><div>// Dst = NewVReg's reg class</div><div>// *II = MCInstrDesc</div><div>// IIOpNum = II Operand Num</div><div><br></div><div>if (TRI->getCommonSubClass(DstRC, TRI->getRegClass(II->OpInfo[IIOpNum].RegClass)) == DstRC)</div><div> MRI->setRegClass(VReg, DstRC);</div><div>else</div><div> BuildMI(... TargetOpcode::COPY...)</div><div><br></div><div>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?</div></div></div></blockquote><div><br></div></span><div>Yes, that’s right.</div><span><br><blockquote type="cite"><div><div dir="ltr"><div> It should find the common subclass of the new register class and the valid reg class of the operand for that machine instruction.</div><div><br></div><div>Unfortunately it looks like that condition is always being set to true (or I'm getting a TON of false positives).</div></div></div></blockquote><div><br></div></span><div>Where does II come from?</div><div>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!</div><div><br></div><div>You want to check "common subclass” of DstRC and SrcRC.</div><div><br></div><div>Cheers,</div><div>Q.</div><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Thanks.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 2:09 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 22, 2015, at 9:10 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>One last question regarding this please.</div><div><br></div><div>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.</div><div><br></div><div>For example, instead of BuildMI(..., TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg), use something like MRI->setRegClass(VReg, MRI->getRegClass(NewVReg)) ?</div></div></div></blockquote><div><br></div></span><div>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.</div><span><font color="#888888"><div><br></div><div>Q.</div></font></span><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>What is the reasoning behind adding the extra instruction instead of changing the reg class? </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 19, 2015 at 2:04 PM, Ryan Taylor <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><div>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).</div><div><br></div><div>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.</div><div><br></div><div>Thanks.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 19, 2015 at 1:32 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Aug 19, 2015, at 9:42 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>It seems the problem arises from using multiple reg classes for one MI in the td file, I guess.</div></div></div></blockquote><div><br></div></span><div>Probably, that does not sound something used widely :).</div><span><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div> 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.</div></div></div></blockquote><div><br></div></span><div>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.</div><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>I just need to create MIs for every possible case I guess. </div><div>Thanks for the help! :)</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 19, 2015 at 12:04 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><div>Hi Ryan,</div><br><div><span><blockquote type="cite"><div>On Aug 19, 2015, at 6:35 AM, Ryan Taylor via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:</div><br><div><div dir="ltr"><div>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?</div></div></div></blockquote><div><br></div></span><div>We match the instructions bottom-up and I believe that copy are automatically inserted when the register classes do not match.</div><div>That seems strange to me that the isel logs are exactly the same but still you are seeing a different result of instruction selection.</div><span><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>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..</div><div><br></div><div>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.</div></div></div></blockquote><div><br></div></span><div>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.</div><div><br></div><div>Anyhow, I’d say debug the isel process (probably the InstrEmitter part) to see where the problem arise.</div><div><br></div><div>Cheers,</div><div>-Quentin</div><br><blockquote type="cite"><div><div><div><div dir="ltr"><div><br></div><div>Thanks.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 17, 2015 at 7:00 PM, Ryan Taylor <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Ryan Taylor</b> <span dir="ltr"><<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>></span><br>Date: Mon, Aug 17, 2015 at 6:59 PM<br>Subject: Re: [LLVMdev] TableGen Register Class not matching for MI in 3.6<br>To: Quentin Colombet <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>><br>Cc: "<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>" <<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>><br><br><br><div dir="ltr"><div>The isel logs are exactly the same, nothing changed, it's matching the same instructions just the reg classes are different.</div><div><br></div><div>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.</div><div><br></div><div>Thanks.</div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 31, 2015 at 1:21 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div><br><div><span><blockquote type="cite"><div>On Jul 31, 2015, at 10:14 AM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div>Quentin,</div><div><br></div><div> 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.</div></div></div></blockquote><div><br></div></span><div>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.</div><div><br></div><div>Sorry for not being more helpful here.</div><span><font color="#888888"><div><br></div><div>Q.</div></font></span><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 31, 2015 at 12:50 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">Hi Ryan,<br>
<br>
Could you check where those moves come from?<br>
<br>
In particular, is this the product of the instruction selection process?<br>
<br>
You use -print-machineinstrs to see when it is inserted.<br>
<br>
Thanks,<br>
-Quentin<br>
<div><div><br>
> On Jul 30, 2015, at 2:02 PM, Ryan Taylor <<a href="mailto:ryta1203@gmail.com" target="_blank">ryta1203@gmail.com</a>> wrote:<br>
><br>
> In LLVM 3.6,<br>
><br>
> 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.<br>
><br>
> For example, I have a mov instruction. mov can be executed between different register types (ie gpr, index, base, etc..)<br>
><br>
> In 3.4, we would get something like this:<br>
><br>
> mov @a, %b1 // moving this immediate to a base register, which is what we want<br>
><br>
> In 3.6, we now get this:<br>
><br>
> mov @a, %r0 // r0 = gpr<br>
> mov %r0, %b1 // b1 = base reg<br>
><br>
> The register class looks like this:<br>
><br>
> def ARegs : RegisterClass<"us", [i16], i16, (add GPRRegs, IndexRegs, BaseRegs)>;<br>
><br>
> I have absolutely no idea why this is not matching any longer?<br>
><br>
> The fix here is to define an MI with explicit single register class (ie it only allows PTRRegs as the destination).<br>
><br>
> 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?<br>
><br>
> Thanks.<br>
</div></div>> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="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=" target="_blank" rel="noreferrer">http://llvm.cs.uiuc.edu</a><br>
> <a href="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=" target="_blank" rel="noreferrer">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br>
</blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></div></div><br></div>
</blockquote></div><br></div>
_______________________________________________<br>LLVM Developers mailing list<br></div></div><a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br><a href="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=" target="_blank">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=</a> <br></div></blockquote></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div>
</div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></div></blockquote></div><br></div>