[PATCH] Fix treatment of ARM unallocated hint instructions

[Mihail Popa]

Sure - sorry for delaying - it'll be in by tomorrow.


On Tue, Apr 23, 2013 at 8:34 PM, Jim Grosbach <grosbach at apple.com> wrote:

> 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/20130424/98e10a43/attachment.html>