<div dir="ltr">Hi Tim.<div><br></div><div>While not beautiful, it's the least ugly solution.</div><div>This can't be handled by custom parsing alone unfortunately.</div><div>We'd have to change how the immediate is represented. Currently</div>
<div>in MC the immediate is still a single number. If we just parse this syntax</div><div>and keep the internal representation of this kind of operand we are</div><div>loosing the ability to differentiate between equivalent yet different encodings.</div>
<div><br></div><div>For example:</div><div><br></div><div>and r0, r1, #1, #0</div><div>and r0, r1, #1, #2</div><div>...</div><div>and r0, r1, #1, #30</div><div><br></div><div>are are equivalent but have different encodings. The point is to retain</div>
<div>the exact form that was parsed. </div><div><br></div><div>This implies that internally MC needs to be changed to hold the [value, modifier]</div><div>pair rather than the value itself. Isn't this uglier, more difficult and more risky?</div>
<div><br></div><div>To me it seems more natural to solve an essentially syntactical problem with a</div><div>asm parsing only solution.</div><div><br></div><div>The other thing is that I have doubts about the asm parser's ability to support this,</div>
<div>even via custom parsing methods. In the end what tablegen generates is a parser</div><div>in name only. It's not a real parser. I suspect that a certain amount of hacking would</div><div>be required to support an immediate operand which consists of a pair of values where</div>
<div>the second element is optional and defaults to zero.</div><div><br></div><div>Regarding the duplication, this is a tablegen improvement opportunity. We should </div><div>be able to factor out the definitions common to all members of a multiclass; but</div>
<div>alas this doesn't work.</div><div><br></div><div>Mihai</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 29, 2013 at 4:31 PM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Mihail,<br>
<div class="im"><br>
> Please review the attached patch. It adds support for "modified immediate"<br>
> syntax form to the relevant ARM instructions. Basically these immediate<br>
> fields are represented via a pair of values: one being a base value and the<br>
> other a modifier.<br>
<br>
</div>A good goal, but I think this is the wrong approach, for two reasons:<br>
1. Copy-pasting a dozen instructions usable only for AsmParsing just<br>
to support this syntax is horrific.<br>
2. It doesn't handle (or extend to handling) the disassembly case<br>
neatly. Some of these instructions *should* print without the extra<br>
immediate (e.g. "movs r11, #99, #0").<br>
<br>
I think this is a perfect candidate for adding a "ParserMethod" to the<br>
so_imm Operand type (well, SOImmAsmOperand)[1]. Obviously<br>
isSOImmAsmOperand would have to change too (probably to become<br>
trivial). Then when you get around to implementing disassembly, you<br>
just need to add a single PrintMethod and you're done.<br>
<br>
Cheers.<br>
<br>
Tim.<br>
<br>
[1] Well, actually I think your new instructions are better than the<br>
existing ones and should be canonical, with the single-immediate<br>
version handled by an InstAlias. Unfortunately, I don't think TableGen<br>
is anywhere near sophisticated enough to handle that neatly yet)<br>
</blockquote></div><br></div>