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
