[PATCH] Fix treatment of ARM unallocated hint instructions

[Jim Grosbach]

Ping. Please fix this sooner rather than later. As-is, the test case is fundamentally wrong and the diagnostic is bogus. The end result is that the behavior after this patch is overall worse than before.

-Jim

On Apr 18, 2013, at 5:19 AM, Mihail Popa <mihail.popa at gmail.com> wrote:

> Jim,
> 
> I will address your concerns in a subsequent patch. 
> You are correct - the same issue is present in the Thumb encoding space as well. 
> I have left that alone simply because ARM encodings are of higher priority at the moment, but will fix in the near future.
> 
> Mihai
> 
> 
> 
> On Wed, Apr 17, 2013 at 11:52 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
> 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/20130423/408f1ddf/attachment.html>