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

[Jim Grosbach]

Hey Richard,

No need to apologize. Figuring this kind of thing out is a part of why we have the review process. I'll have a look at the new patch once I'm back at my workstation.

-Jim

On Apr 27, 2012, at 10:53 AM, Richard Barton <richard.barton at arm.com> wrote:

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