Agreed. I’m fine with the idea, but don’t know much about how fixits are implemented.
-jim > On Jul 25, 2014, at 11:20 AM, Eric Christopher <[email protected]> wrote: > > 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 <[email protected]> wrote: >> Hi Eric, >> >> Partial revert done in r213965. >> >> Cheers, >> >> James >> >> -----Original Message----- >> From: Eric Christopher [mailto:[email protected]] >> Sent: 24 July 2014 18:35 >> To: James Molloy >> Cc: Jim Grosbach; [email protected] >> 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 <[email protected]> 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:[email protected]] >>> Sent: 18 July 2014 21:16 >>> >>> >>> To: James Molloy >>> Cc: [email protected] >>> 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 <[email protected]> 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: [email protected] >>> 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 <[email protected]> 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 >>>> [email protected] >>>> 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 >>> [email protected] >>> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
