[PATCH] Fix treatment of ARM unallocated hint instructions

[Jim Grosbach]

Thanks, much clearer now. This is a nice tightening up of things.

Also, thanks for reviewing this Quentin. I just a have a few additional comments on some of the details.

+def Imm0_4AsmOperand : ImmAsmOperand { let Name = "Imm0_4"; }

This should have a custom DiagnosticType as well to help get better error messages for out of range values. We don’t use this facility as much as we should in the ARM backend, but it’s worth doing. Imm0_15AsmOperand has an example of how this works.


-def HINT : AI<(outs), (ins imm0_255:$imm), MiscFrm, NoItinerary,
+def HINT : AI<(outs), (ins imm0_4:$imm), MiscFrm, NoItinerary,
               "hint", "\t$imm", []>, Requires<[IsARM, HasV6]> {
-  bits<8> imm;
-  let Inst{27-8} = 0b00110010000011110000;
-  let Inst{7-0} = imm;
+  bits<3> imm;
+  let Inst{27-3} = 0b0011001000001111000000000;
+  let Inst{2-0} = imm;
 }

This is only for the ARM mode of the instruction. Don’t restrictions like this also apply to the Thumb2 encodings?

 @ CHECK: yield                          @ encoding: [0x01,0xf0,0x20,0xe3]
 @ CHECK: yieldne                        @ encoding: [0x01,0xf0,0x20,0x13]
-@ CHECK: hint	#5                      @ encoding: [0x05,0xf0,0x20,0xe3]
+@ CHECK-NOT: hint	#5
 @ CHECK: sev    

A better way to handle this is that “hint #5” should be removed entirely from this test file since it’s not a valid encoding. It should be replaced with test cases in diagnostics.s and thumb-diagnostics.s which check that the expected diagnostic is produced. As is, for example, we get "error: instruction requires: thumb2” which isn’t correct, since this instruction shouldn’t be valid for thumb2 either. We should explicitly check for the new diagnostic we add via DiagnosticType.

Jim


On Apr 17, 2013, at 3:10 AM, Mihail Popa <mihail.popa at gmail.com> wrote:

> Hi.
> 
> I have amended the patch by removing white space trimming and replacing grep with FileCheck.
> 
> Regards,
> Mihai
> 
> 
> On Tue, Apr 16, 2013 at 7:47 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> Oh yes, that’s true. I’ve forgotten about that!
> 
> -Quentin
> 
> On Apr 16, 2013, at 11:36 AM, Tim Northover <t.p.northover at gmail.com> wrote:
> 
>>> Maybe I’m missing something but you’ve defined a new isImm0_4 method but
>>> you’re never using it.
>> 
>> That's automatically used by TableGen based on the Name member in the
>> AsmOperandClass, isn't it?
>> 
>> Tim.
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> <LLVM-713.hints.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130417/a3b37c9e/attachment.html>