[llvm-commits] [PATCH] thumb t2b fix

[Greg Fitzgerald]

Thanks for the review Tim.  Just to verify, when you say "MCInst's
immediate", we should distinguish the value returned by "getImm()"
from the value in 'target' in the .td file.  I believe getImm() should
return the sign-extended 'imm32' and that 'target' can contain
'S:J1:J2:imm10:imm11'.  Otherwise, you're asking the user to do
redundant bit-twiddling during instruction selection and relaxation,
whereas the existing code simply modifies the opcode and lets the
encoding pass muck with the bits.  In my patch, 'target' contains
'S:I1:I2:imm10:imm11' (no J1/J2 yet and no trailing zero).  Then, the
dirty part, the encoder/decoder methods swap I1/I2 and J1/J2.

My interpretation of Owen's suggestion is to have a symmetric
encoder/decoder on 'target' and remove the decoder on the 't2B'
definition.  As he says, this lets the .td file simply blast the bits
to the right locations.  My patch does very close to that but I wanted
to keep the patch as small as possible, as well as keep consistent
with the 't2Bcc' definition.  My preference, generally speaking, is to
go "minimally invasive" when fixing a bug, and then refactor as a
separate "cleanup" patch.  One might expect the first patch to always
come with a unit test and the second to demonstrate breaking no
existing tests.  Sound good?

Thanks,
Greg


On Thu, Sep 20, 2012 at 12:09 AM, Tim Northover <t.p.northover at gmail.com> wrote:
> On Wed, Sep 19, 2012 at 11:21 PM, Greg Fitzgerald <garious at gmail.com> wrote:
>> Okay, here's my first shot.  It starts with Christopher's patch and
>> tests, adds a couple more tests, removes the bit-clearing code, and I
>> believe fixes a bug in the fixup encoding.
>
> I believe the fix is correct, but it doesn't address Owen's preference
> for the MCInst's immediate to be S:J1:J2:imm10:imm11. We're also
> working on the more complete solution, but can probably cope with any
> merge conflicts if this does get committed as an interim measure.
>
> Tim.