[llvm-commits] [PATCH] thumb t2b fix

[Tim Northover]

Hi Greg,

> 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'.

I believe that "target" only really exists in the .td file and cannot
be anything but S:J1:J2:imm10:imm11 by definition (it's assigned
directly to Inst{}!). (Intepreting the words of the prophet ;-)) I
think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.
He specifically refers to the MCOperand as having that value.

> Otherwise, you're asking the user to do
> redundant bit-twiddling during instruction selection and relaxation,

At a casual glance, selection is likely fine (it's just going to give
a label with an attached fixup: getImm() == 0 is acceptable).
Relaxation has to account for all the complexity regardless of which
route is taken.

The other relevant bits are:
+ Disassembler: complex now; no special handling in the proposed solution.
+ InstPrinter: simple now; more complex in proposed solution.
+ Encoder: complex now; simple in proposed solution.
+ AsmParser: simple now; complex in proposed solution.

We're shuffling the complexity around to achieve two goals:
1. Fits in with other instructions in having operands to an MCInst
already encoded where possible.
2. Make and MCInst is intrinsically valid (not entirely convinced on
this one, but it's certainly a step closer to that goal than the
status quo).

> My interpretation of Owen's suggestion is to have a symmetric
> encoder/decoder on 'target' and remove the decoder on the 't2B'
> definition.

I'm really not sure what you mean by "target"here. The only reference
I see is an arbitrary field name in the .td file. During runtime the
primary place this "target" exists is as an MCOperand. Various
components have to either create or interpret that value.

> My preference, generally speaking, is to go "minimally invasive" when fixing a
> bug, and then refactor as a separate "cleanup" patch.[...] Sound good?

I obviously have no problem with the first part of that, but I think
you're misinterpreting Owen's goals for the long term.

Tim.