<div>My last patch wasn't accepted, I believe, because it didn't pre-encode the immediate in the MC instruction.  Unlike my ADR patch, in this patch, it was more obvious that pre-encoding isn't a good choice because of the extra bit-twiddling.  That previous patch can no longer be merged cleanly because of changes, but I notice that some encodings are still broken.  Here's a patch that only updates and XFAIL's the unit-tests.  If you are interested in a patch that doesn't pre-encode the immediate, I'd be happy to update my previous patch to work with the latest code.  Otherwise, I hope you might accept that small patch that helps track the existing issue.</div>

<div><br></div><div><a href="https://github.com/garious/llvm/commit/916c4badd816178da9fdbac5b5ed2331a7201f98">https://github.com/garious/llvm/commit/916c4badd816178da9fdbac5b5ed2331a7201f98</a></div><div><br></div><div>Thanks,</div>

<div>Greg</div><div><br><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 27, 2012 at 10:00 AM, Greg Fitzgerald <span dir="ltr"><<a href="mailto:garious@gmail.com" target="_blank">garious@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ping.<br>
<br>
same patch attached<br>
<div class="HOEnZb"><div class="h5"><br>
On Wed, Sep 26, 2012 at 10:27 AM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>> wrote:<br>
> The attached patch is the same as before but includes a missing<br>
> function declaration.<br>
><br>
> Thanks,<br>
> Greg<br>
><br>
><br>
> On Mon, Sep 24, 2012 at 5:15 PM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>> wrote:<br>
>>> I believe that "target" only really exists in the .td file and cannot<br>
>>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned<br>
>>> directly to Inst{}!).<br>
>><br>
>> 'target' maps to the input value to the decoder method and the return<br>
>> value of the encoder method.<br>
>><br>
>><br>
>>> I think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.<br>
>>> He specifically refers to the MCOperand as having that value.<br>
>><br>
>> Since no instructions in the ARM backend are currently written this<br>
>> way, when making bug fixes, can we aim to be consistent with the<br>
>> existing code?<br>
>><br>
>> The more I think about it, I'm really not on board with the long-term<br>
>> plan of exposing the encoding details to the caller of getImm().  I<br>
>> think that method should return 'imm32', not 'S:J1:J2:imm10:imm11'.<br>
>> Returning imm32 keeps relaxation and instruction selection simple.<br>
>> But in any case, can we please move this aspect of the discussion to<br>
>> another thread?  If we're going to go this route, we should make the<br>
>> change to all ARM instructions at once, and as a separate patch.<br>
>><br>
>> Attached is an updated patch ready for review.  It corrects an<br>
>> encoding bug in the J1/J2 bits, adds more unit tests, and simplifies<br>
>> the code by making a symmetric encoder/decoder for 'target'.<br>
>><br>
>> Thanks,<br>
>> Greg<br>
>><br>
>><br>
>><br>
>> On Thu, Sep 20, 2012 at 1:20 PM, Tim Northover <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>> wrote:<br>
>>> Hi Greg,<br>
>>><br>
>>>> Thanks for the review Tim.  Just to verify, when you say "MCInst's<br>
>>>> immediate", we should distinguish the value returned by "getImm()"<br>
>>>> from the value in 'target' in the .td file.  I believe getImm() should<br>
>>>> return the sign-extended 'imm32' and that 'target' can contain<br>
>>>> 'S:J1:J2:imm10:imm11'.<br>
>>><br>
>>> I believe that "target" only really exists in the .td file and cannot<br>
>>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned<br>
>>> directly to Inst{}!). (Intepreting the words of the prophet ;-)) I<br>
>>> think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.<br>
>>> He specifically refers to the MCOperand as having that value.<br>
>>><br>
>>>> Otherwise, you're asking the user to do<br>
>>>> redundant bit-twiddling during instruction selection and relaxation,<br>
>>><br>
>>> At a casual glance, selection is likely fine (it's just going to give<br>
>>> a label with an attached fixup: getImm() == 0 is acceptable).<br>
>>> Relaxation has to account for all the complexity regardless of which<br>
>>> route is taken.<br>
>>><br>
>>> The other relevant bits are:<br>
>>> + Disassembler: complex now; no special handling in the proposed solution.<br>
>>> + InstPrinter: simple now; more complex in proposed solution.<br>
>>> + Encoder: complex now; simple in proposed solution.<br>
>>> + AsmParser: simple now; complex in proposed solution.<br>
>>><br>
>>> We're shuffling the complexity around to achieve two goals:<br>
>>> 1. Fits in with other instructions in having operands to an MCInst<br>
>>> already encoded where possible.<br>
>>> 2. Make and MCInst is intrinsically valid (not entirely convinced on<br>
>>> this one, but it's certainly a step closer to that goal than the<br>
>>> status quo).<br>
>>><br>
>>>> My interpretation of Owen's suggestion is to have a symmetric<br>
>>>> encoder/decoder on 'target' and remove the decoder on the 't2B'<br>
>>>> definition.<br>
>>><br>
>>> I'm really not sure what you mean by "target"here. The only reference<br>
>>> I see is an arbitrary field name in the .td file. During runtime the<br>
>>> primary place this "target" exists is as an MCOperand. Various<br>
>>> components have to either create or interpret that value.<br>
>>><br>
>>>> My preference, generally speaking, is to go "minimally invasive" when fixing a<br>
>>>> bug, and then refactor as a separate "cleanup" patch.[...] Sound good?<br>
>>><br>
>>> I obviously have no problem with the first part of that, but I think<br>
>>> you're misinterpreting Owen's goals for the long term.<br>
>>><br>
>>> Tim.<br>
</div></div></blockquote></div><br></div>