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

[Richard Barton]

Hi Jim 

Firstly I would like to issue an apology. My first e-mail contained a lot of
blatant fallacies that I should have checked up on better beforehand.

More comments inline...

> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 25 April 2012 17:15
> To: Richard Barton
> Cc: llvm-commits at cs.uiuc.edu LLVM; Bill Wendling; Owen Anderson; Kevin Enderby
> Subject: Re: [PATCH] Implement unallocated NOP Hints
> 
> 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.
> >

This is my mistake. For pre-v6T2 architectures, these encodings are, in fact
undefined, so MC is correct in pre-v6T2 state for all such instructions except
for the YIELD, SEV, WFI, and WFE hints.

The conditionalisation was missing for these instructions, the trivial patch is
attached [pre_v6T2_YIELD_et_al.patch], please review.


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

This was my interpretation of the ARMARM. I have consulted with some local
experts and your interpretation is correct. So MC is correct for these
instructions in v6T2 and later. Sorry about that. 

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

Thanks for explaining the philosophy to me. I now understand that tablegen
describes only instructions that can have a assembler format, and that there is
a distinction from instructions that have only disassembly representations.

I don't object to a change in the philosophy either and if I understand your
suggestion correctly this would be not too large a change to make. 

However, I don't think that these instructions are worth changing the philosophy
for right now. In terms of priority for ARM, this sits rather lower in our
priority than other code generation problems that might be present in MC. 

We are tracking this issue locally, I can raise a public bug report on Monday
morning. Do you think this would be worthwhile? 

The current behaviour (disassembling and re-encoding to an unpredictable 'IT EQ'
upsets me in that it can have ramifications on the next instruction in the code.
However I accept that changing to any other wrong behaviour (emitting a nop for
example) is no better than leaving the current wrong behaviour, and that this
situation is likely to be a very rare occurrence anyway.

So in summary, I will retract my original patch as unsuitable (with the
discovery that the unallocated hints are undefined in pre-v6T2 it now does not
work anyway.) I am confident that the smaller patch attached to this issue is
correct and worthwhile applying.

It is my fault for raising this issue really, but at least one genuine bug fix
has come out of it.

Thanks
Rich



> > 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>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pre_v6T2_YIELD_et_al.patch
Type: application/octet-stream
Size: 3882 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120427/1193b3cf/attachment.obj>