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
Mon Aug 11 23:49:51 PDT 2014 

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 <ahatanak at gmail.com> 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 <bob.wilson at apple.com> wrote:
>>
>>
>> On 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 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: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?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
>> > 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
>> <asm-modifiers.diff>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

More information about the cfe-commits mailing list