[llvm-commits] [PATCH] Implement unallocated NOP Hints

[Jim Grosbach]

Hi Richard,

Thanks for looking at this. We (obviously) haven't really done much for this sort of thing in the ARM backend so far. It'll be great to make progress here.

That said, I'm a bit concerned about the specific approach. Comments below.

-Jim

On Apr 25, 2012, at 5:33 AM, Richard Barton <richard.barton at arm.com> wrote:

> Hi Bill
> 
> They keep coming... another IT patch. I have cc-ed Jim as he has been helping
> with these types of code reviews recently.
> 
> The attached patch implements unallocated NOP Hints in MC. These are 16-bit
> Thumb instructions with encoding:
> 
> 15     12      8       4       0
> |1,0,1,1|1,1,1,1|  opA  |  opB  |
> 
> [the below table is from: ARMARM 6.2.5. Miscellaneous 16-bit instructions]
> 
> Table 6.7. 16-bit If-Then and hint instructions
> opA	opB		Instruction			See	Variant
> -	not 0000	If-Then			IT	v6T2
> 0000	0000		No Operation hint		NOP	v6T2
> 0001	0000		Yield hint			YIELD	v7
> 0010	0000		Wait For Event hint	WFE	v7
> 0011	0000		Wait For Interrupt hint	WFI	v7
> 0100	0000		Send Event hint		SEV	v7
> 
> current MC Behaviour
> ---------------------
> For pre-v6T2:
> - YIELD, SEV, WFI, WFE instructions incorrectly decode to v6T2 versions. 

That's odd. Sounds like the predicates aren't set up right, maybe?

> - NOP incorrectly decode as undefined instructions.
> - IT was incorrectly decode as undefined instructions. 
> - Any other hints incorrectly decode as undefined instructions.
> 
> For v6T2 and later:
> - YIELD, etc. instructions decode (this is correct for v7 but not for v6T2)

Why is this incorrect? According to the docs I have, the assembly syntax is fine for both v7 and v6t2, they just execute as a NOP on v6t2.

>  - NOP correctly decode.
> - IT correctly decode.
> - Any other hints incorrectly decode as unpredictable IT instructions.
> 
> My patch adds a new tablegen class to match these only for pre-v6T2. These will
> disassemble as: 
> "nop @ UNALLOCATED opA = #<num> opB = #<num>"
> 
> This is mildly unpleasant in that it adds a dependency on the comment character.
> It will can also never be assembled, but this is fine as the ARMARM says
> "software must not use them." 
> 

That's a bit of a problem, unfortunately. The tblgen files are intended, as it stands now anyway, to describe all legal instructions, not to describe the entire encoding space. This patch, if I follow it right, is changing that approach. I don't have a problem with that conceptually, but I would like to have a clearer high level design for how to deal with that in the general sense before diving in to the specifics too much. It wouldn't need to be a huge departure from the current stuff, but we would need a way to communicate to the system the semantics of what we're describing. That is, distinguish between real instructions and disassembly presentation of unallocated encodings. Even something as simple as a bit on the Instruction class denoting which is which might be sufficient.

The sort of thing I want to be able to do is avoid putting these instructions in the assembler matcher tables if they're not something that can ever be matched.

> I opted for this approach as it allows the opcode fields to be expressed in the
> tablegen as Operand types, which means that there is no need for a specialist
> decoder and encoder.

I don't think there will need to be a special encoder regardless. A decoder hook would be nice to avoid, but it doesn't worry me overmuch.

> Patched MC behaviour
> --------------------
> For pre-v6T2:
> - All such instructions are decoded as unallocated NOPs, and disassembled as
> shown.
> 
> For v6T2:
> - NOP and IT behaviour unchanged
> - YIELD, SEV, etc. decoded as predictable, unallocated NOPs and disassembled as
> shown.
> - All other instructions decoded as predictable, unallocated NOPs and
> disassembled as shown.
> 
> For V7 and later:
> - NOP and IT behaviour unchanged
> - YIELD, SEV, etc. behaviour unchanged from old v6T2 and later behaviour
> - All other instructions decoded as predictable, unallocated NOPs and
> disassembled as shown.
> 
> In order to do this I had to implement the NoV6T2 predicate so that tablegen
> could output an if statement in the disassembler to correspond to it.
> 
> My patch also updates tests that incorrectly disassemble yield, sev, etc for
> thumb1.
> 
> Please review.
> 
> Thanks,
> Richard Barton
> ARM Ltd, Cambridge
> 
> <unallocated_nops_better.patch>