<div dir="ltr">Tim,<div><br></div><div>changing the representation of these immediate operands breaks frame lowering.</div><div>There is a lot of code along the lines of:</div><div><br></div><div><font face="courier new, monospace"> AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tMOVr), ARM::R4)</font></div>
<div><font face="courier new, monospace"> .addReg(ARM::SP, RegState::Kill));</font></div><div><font face="courier new, monospace"> AddDefaultCC(AddDefaultPred(BuildMI(MBB, MBBI, dl,</font></div><div><font face="courier new, monospace"> TII.get(ARM::t2BICri), ARM::R4)</font></div>
<div><font face="courier new, monospace"> .addReg(ARM::R4, RegState::Kill)</font></div><div><font face="courier new, monospace"> <font color="#ff0000">.addImm</font>(MaxAlign-1)));</font></div>
<div><font face="courier new, monospace"> AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tMOVr), ARM::SP)</font></div><div><font face="courier new, monospace"> .addReg(ARM::R4, RegState::Kill));</font></div>
<div> </div><div>There are a number of places in the backend where MachineInstructions are created this way. Since most</div><div>code deals with adds, subs and alignment, most addImm calls should be changed. addImm calls alone</div>
<div>are around 300. In addition to that, some instructions get modified and will have to be resized, adding an</div><div>additional operand.</div><div><br></div><div>The best solution I can think of is to derive a class out of MachineInstructionBuilder which will provide methods</div>
<div>for creating two operand immediates. The issue still remains of reviewing each and every addImm in there.</div><div><br></div><div>The effort is not the issue here. Do you really think it's work the risk? Do you really think this is a better and neater </div>
<div>solution? If you do, I'll do it - but I must say that the original patch I submitted is the solution that makes more</div><div>sense simply because it keeps changes local. </div><div><br></div><div>The scope of these changes are simply much beyond the scope of the benefit to warrant implementation.</div>
<div>However, you decide. Let me know.</div><div><br></div><div>Regards,</div><div>Mihai</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 13, 2013 at 3:25 PM, Tim Northover <span dir="ltr"><<a href="mailto:tnorthover@apple.com" target="_blank">tnorthover@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 class="im">On 13 Aug 2013, at 14:36, Mihail Popa <<a href="mailto:mihail.popa@gmail.com">mihail.popa@gmail.com</a>> wrote:<br>
> Is there any proper documentation for what ComplexPatterns do and how they interact with<br>
> the rest of the descriptions in the td file? Surprisingly, Google is not my friend today<br>
<br>
</div><div class="im">> Specifically why do I get error in pattern definitions once I add MIOperandInfo? Ex:<br>
<br>
</div>I think both of your errors are because of incompatibility between so_imm, so_imm_neg and so_imm_not.<br>
<br>
The SUB pattern is trying to use so_imm_neg to match an instruction defined with (ins so_imm:$imm) and the MVN similarly has a pattern in the instruction using so_imm_not.<br>
<br>
Both of the others are still expecting to produce a single immediate, rather than two. Hence the problem.<br>
<br>
Unfortunately ComplexPatterns are even less well-documented than many other parts of TableGen (and have weird and wonderful restrictions on how they can be applied). I think the best is include/llvm/Target/TargetSelectionDAG.td. Other than that it's the TableGen source.<br>
<div class="im"><br>
> But here is the problem: since this kind of immediate may be affected by fixups, it will be potentially<br>
> modified outside the encoder/decoder/printer/render methods which are tied to the operand class.<br>
> Thus I actually need to know based on an opcode:<br>
<br>
</div>What do you see modifying the operand elsewhere that needs to know those details?<br>
<br>
I think AArch64's MOVZ/MOVK instructions implement a scheme very similar to what I'm describing, and need fixups too. It looks like, in the case where a fixup is needed, I made the imm16 part reference the symbol (and hence get the fixup) and put a plain 0 imm into the shift amount.<br>
<div class="im"><br>
> Also isn't a bitwise representation closer to the "policy" of representing operands as close<br>
> to the encoding as possible?<br>
<br>
</div>I don't think so. The fields happen to be listed as "imm12" but it's clear as soon as you read the description that it's really two separate fields. Especially once the general syntax comes into play.<br>
<div class="im"><br>
> Surely representing a floating point as a <sign, mantissa, exp> isn't any more accurate.<br>
<br>
</div>Now that has merit. Though floats have a history of being a generally recognised primitive type, which I don't think applies here.<br>
<div class="im"><br>
> Additionally, it does not matter which encoding gets generated for selected code.<br>
> I only need to preserve the exact representation for assembled code...<br>
<br>
</div>I think that's equally easy with bitwise or two separate operands.<br>
<br>
In the end, though, I'm not going to reject a patch that puts everything into a single immediate. If you really think that's neater (and not just easier) then go ahead, you're the one doing the work after all.<br>
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div>