The inline asm changes look fine. I expect you'll want to ask Richard Smith about the rest of the changes.
-eric On Mon, Aug 11, 2014 at 2:42 PM, Akira Hatanaka <[email protected]> wrote: > The attached patch improves upon the patch James sent out a few weeks ago. > > It changes validateConstraintModifier to return the suggested modifier and > also makes changes to print the caret and the suggested modifier right below > the operand in the assembly template. > > > > On Fri, Aug 1, 2014 at 11:53 AM, Bob Wilson <[email protected]> wrote: >> >> >> On 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 think it would usually be more helpful to suggest a fixit that adds a >> “w” or “x” modifier to the operand constraint. Everyone knows how to cast >> the operand but many people don’t know about the modifiers. >> >> 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?rev=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-inline-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-validate.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 >> <asm-modifiers.diff>_______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
