r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
Eric Christopher
echristo at gmail.com
Fri Jul 25 11:20:06 PDT 2014
I guess the idea of a fixit is ok, I'd talk to Richard about the best
way to plumb fixits through things.
-eric
On Fri, Jul 25, 2014 at 3:28 AM, James Molloy <James.Molloy at arm.com> wrote:
> Hi Eric,
>
> Partial revert done in r213965.
>
> Cheers,
>
> James
>
> -----Original Message-----
> From: Eric Christopher [mailto:echristo at gmail.com]
> Sent: 24 July 2014 18:35
> To: James Molloy
> Cc: Jim Grosbach; cfe-commits at cs.uiuc.edu
> Subject: Re: r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
>
> Hi James,
>
> It'd be much easier if you did it as two separate patches, the revert and the new patch for the functionality you like. The revert is obviously approved to go in any time :)
>
> Thanks!
>
> -eric
>
> On Thu, Jul 24, 2014 at 7:28 AM, James Molloy <James.Molloy at arm.com> wrote:
>> Hi Jim,
>>
>>
>>
>> Thanks for your patience on this.
>>
>>
>>
>> I’ve gone back and taken another look at this, and I believe you’re
>> totally right. GCC will invisibly promote whatever you give it to
>> 64-bits and emit an X-reg instruction. I agree that we want a warning here.
>>
>>
>>
>> However, I think the warning as it was pre-r206963 could be confusing
>> for the user. It is not immediately obvious, for example when using
>> the integer literal “0”, what the user has done wrong (here, they’d
>> need “0L”). So the attached patch:
>>
>> · Reverts the main functionality change of r206963 (w.r.t
>> modifiers).
>>
>> · Modifies TargetInfo to allow the modifier validation hook to
>> inform Sema about a fixit hint to insert.
>>
>> · Implements this in AArch64TargetInfo to display a fixit.
>>
>>
>>
>> The fixit will be a cast of the asm operand to the appropriate type.
>> I’m not thoroughly happy with the changes to TargetInfo – it’s quite
>> restrictive to only communicate via a std::string (type to fixit-cast
>> to). However, TargetInfo is in Basic and so can’t access clang::Type L
>>
>>
>>
>> Suggestions to better approaches would be appreciated!
>>
>>
>>
>> Cheers,
>>
>>
>>
>> James
>>
>>
>>
>> From: Jim Grosbach [mailto:grosbach at apple.com]
>> Sent: 18 July 2014 21:16
>>
>>
>> To: James Molloy
>> Cc: cfe-commits at cs.uiuc.edu
>> Subject: Re: r206963 - [ARM64] Change inline assembly constraints to
>> be more lax, to match the behaviour of Clang/AArch64 and GCC.
>>
>>
>>
>> Hi James,
>>
>>
>>
>> That’s perfectly fine. It’s important to get resolved, but not super urgent.
>>
>>
>>
>> Thanks,
>>
>> Jim
>>
>>
>>
>> On Jul 18, 2014, at 1:00 PM, James Molloy <james.molloy at arm.com> wrote:
>>
>>
>>
>> Hi Jim,
>>
>> Understood - I'm on vacation until Tuesday, and I'll need to have a
>> chat with others about the right way forward here. Is delaying this
>> until Tuesday OK with you?
>>
>> Cheers,
>> James
>>
>> ________________________________
>>
>> From: Jim Grosbach
>> Sent: 18/07/2014 19:57
>> To: James Molloy
>> Cc: cfe-commits at cs.uiuc.edu
>> Subject: Re: r206963 - [ARM64] Change inline assembly constraints to
>> be more lax, to match the behaviour of Clang/AArch64 and GCC.
>>
>> Hi James,
>>
>> I’d like to revisit this. There were objections raised when this first
>> went in that were never resolved. This warning was added because it
>> actively found bugs, not for a theoretical concern. The lack of this
>> error is now actively causing problems. I would very much like to see
>> this reverted entirely. At the very least, please re-implement this
>> and keep the old behavior, including the diagnostic, for Darwin. If
>> other platforms want to be more flexible and allow potentially broken
>> code to go through undiagnosed, on their own heads be it.
>>
>> -Jim
>>
>>> On Apr 23, 2014, at 3:26 AM, James Molloy <james.molloy at arm.com> wrote:
>>>
>>> Author: jamesm
>>> Date: Wed Apr 23 05:26:19 2014
>>> New Revision: 206963
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=206963&view=rev
>>> Log:
>>> [ARM64] Change inline assembly constraints to be more lax, to match
>>> the behaviour of Clang/AArch64 and GCC.
>>>
>>> GCC allows sub-64bit values to use the 'r' register constraint.
>>>
>>> Modified:
>>> cfe/trunk/lib/Basic/Targets.cpp
>>> cfe/trunk/test/CodeGen/aarch64-inline-asm.c
>>> cfe/trunk/test/Sema/arm64-inline-asm.c
>>> cfe/trunk/test/Sema/inline-asm-validate.c
>>>
>>> Modified: cfe/trunk/lib/Basic/Targets.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?r
>>> ev=206963&r1=206962&r2=206963&view=diff
>>>
>>> =====================================================================
>>> =========
>>> --- cfe/trunk/lib/Basic/Targets.cpp (original)
>>> +++ cfe/trunk/lib/Basic/Targets.cpp Wed Apr 23 05:26:19 2014
>>> @@ -4643,46 +4643,37 @@ public:
>>> case 'w': // Floating point and SIMD registers (V0-V31)
>>> Info.setAllowsRegister();
>>> return true;
>>> + case 'I': // Constant that can be used with an ADD instruction
>>> + case 'J': // Constant that can be used with a SUB instruction
>>> + case 'K': // Constant that can be used with a 32-bit logical
>>> instruction
>>> + case 'L': // Constant that can be used with a 64-bit logical
>>> instruction
>>> + case 'M': // Constant that can be used as a 32-bit MOV immediate
>>> + case 'N': // Constant that can be used as a 64-bit MOV immediate
>>> + case 'Y': // Floating point constant zero
>>> + case 'Z': // Integer constant zero
>>> + return true;
>>> + case 'Q': // A memory reference with base register and no offset
>>> + Info.setAllowsMemory();
>>> + return true;
>>> + case 'S': // A symbolic address
>>> + Info.setAllowsRegister();
>>> + return true;
>>> + case 'U':
>>> + // Ump: A memory address suitable for ldp/stp in SI, DI, SF
>>> + and DF
>>> modes, whatever they may be
>>> + // Utf: A memory address suitable for ldp/stp in TF mode,
>>> + whatever
>>> it may be
>>> + // Usa: An absolute symbolic address
>>> + // Ush: The high part (bits 32:12) of a pc-relative symbolic
>>> address
>>> + llvm_unreachable("FIXME: Unimplemented support for bizarre
>>> constraints");
>>> case 'z': // Zero register, wzr or xzr
>>> Info.setAllowsRegister();
>>> return true;
>>> case 'x': // Floating point and SIMD registers (V0-V15)
>>> Info.setAllowsRegister();
>>> return true;
>>> - case 'Q': // A memory address that is a single base register.
>>> - Info.setAllowsMemory();
>>> - return true;
>>> }
>>> return false;
>>> }
>>>
>>> - virtual bool validateConstraintModifier(StringRef Constraint,
>>> - const char Modifier,
>>> - unsigned Size) const {
>>> - // Strip off constraint modifiers.
>>> - while (Constraint[0] == '=' || Constraint[0] == '+' || Constraint[0]
>>> == '&')
>>> - Constraint = Constraint.substr(1);
>>> -
>>> - switch (Constraint[0]) {
>>> - default:
>>> - return true;
>>> - case 'z':
>>> - case 'r': {
>>> - switch (Modifier) {
>>> - case 'x':
>>> - case 'w':
>>> - // For now assume that the person knows what they're
>>> - // doing with the modifier.
>>> - return true;
>>> - default:
>>> - // By default an 'r' constraint will be in the 'x'
>>> - // registers.
>>> - return (Size == 64);
>>> - }
>>> - }
>>> - }
>>> - }
>>> -
>>> virtual const char *getClobbers() const { return ""; }
>>>
>>> int getEHDataRegisterNumber(unsigned RegNo) const {
>>>
>>> Modified: cfe/trunk/test/CodeGen/aarch64-inline-asm.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/aarch64-in
>>> line-asm.c?rev=206963&r1=206962&r2=206963&view=diff
>>>
>>> =====================================================================
>>> =========
>>> --- cfe/trunk/test/CodeGen/aarch64-inline-asm.c (original)
>>> +++ cfe/trunk/test/CodeGen/aarch64-inline-asm.c Wed Apr 23 05:26:19
>>> +++ 2014
>>> @@ -1,4 +1,5 @@
>>> // RUN: %clang_cc1 -triple aarch64-none-linux-gnu -emit-llvm -o - %s
>>> | FileCheck %s
>>> +// RUN: %clang_cc1 -triple arm64-none-linux-gnu -emit-llvm -o - %s |
>>> FileCheck %s
>>>
>>> // The only part clang really deals with is the lvalue/rvalue //
>>> distinction on constraints. It's sufficient to emit llvm and make
>>>
>>> Modified: cfe/trunk/test/Sema/arm64-inline-asm.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/arm64-inline-
>>> asm.c?rev=206963&r1=206962&r2=206963&view=diff
>>>
>>> =====================================================================
>>> =========
>>> --- cfe/trunk/test/Sema/arm64-inline-asm.c (original)
>>> +++ cfe/trunk/test/Sema/arm64-inline-asm.c Wed Apr 23 05:26:19 2014
>>> @@ -1,8 +1,9 @@
>>> // RUN: %clang_cc1 -triple arm64-apple-ios7.1 -fsyntax-only -verify
>>> %s
>>> +// expected-no-diagnostics
>>> +
>>> void foo() {
>>> asm volatile("USE(%0)" :: "z"(0LL));
>>> asm volatile("USE(%x0)" :: "z"(0LL));
>>> asm volatile("USE(%w0)" :: "z"(0));
>>>
>>> - asm volatile("USE(%0)" :: "z"(0)); // expected-warning {{value
>>> size does not match register size specified by the constraint and
>>> modifier}} }
>>>
>>> Modified: cfe/trunk/test/Sema/inline-asm-validate.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/inline-asm-va
>>> lidate.c?rev=206963&r1=206962&r2=206963&view=diff
>>>
>>> =====================================================================
>>> =========
>>> --- cfe/trunk/test/Sema/inline-asm-validate.c (original)
>>> +++ cfe/trunk/test/Sema/inline-asm-validate.c Wed Apr 23 05:26:19
>>> +++ 2014
>>> @@ -1,8 +1,9 @@
>>> // RUN: %clang_cc1 -triple arm64-apple-macosx10.8.0 -fsyntax-only
>>> -verify %s
>>> +// expected-no-diagnostics
>>>
>>> unsigned t, r, *p;
>>>
>>> int foo (void) {
>>> - __asm__ __volatile__( "stxr %w[_t], %[_r], [%[_p]]" : [_t] "=&r" (t)
>>> : [_p] "p" (p), [_r] "r" (r) : "memory"); // expected-warning {{value
>>> size does not match register size specified by the constraint and
>>> modifier}}
>>> + __asm__ __volatile__( "stxr %w[_t], %[_r], [%[_p]]" : [_t] "=&r" (t)
>>> : [_p] "p" (p), [_r] "r" (r) : "memory");
>>> return 1;
>>> }
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>> -- IMPORTANT NOTICE: The contents of this email and any attachments
>> are confidential and may also be privileged. If you are not the
>> intended recipient, please notify the sender immediately and do not
>> disclose the contents to any other person, use it for any purpose, or
>> store or copy the information in any medium. Thank you.
>>
>> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>> Registered in England & Wales, Company No: 2557590 ARM Holdings plc,
>> Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in
>> England & Wales, Company No: 2548782
>>
>>
>>
>>
>> -- IMPORTANT NOTICE: The contents of this email and any attachments
>> are confidential and may also be privileged. If you are not the
>> intended recipient, please notify the sender immediately and do not
>> disclose the contents to any other person, use it for any purpose, or
>> store or copy the information in any medium. Thank you.
>>
>> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>> Registered in England & Wales, Company No: 2557590 ARM Holdings plc,
>> Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in
>> England & Wales, Company No: 2548782
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
More information about the cfe-commits
mailing list