[clang] model 'p' inline asm constraint as reading memory

[nickdesaulniers]

Summary

GCC doesn't CSE inline asm when 'p' is used on inputs, neither should
clang. In order to do so, we must not set memory(none) modref on such
inline asm.

If you're passing the address of a variable into inline asm as an
operand, you're going to be reading memory!

Link: https://lore.kernel.org/lkml/[e-mail]/

[efriedma]

I think this is going to change what inputs Sema will accept for "p". If that's intentional, please add test coverage. Otherwise, please make a narrower change.

[nickdesaulniers]

In D145416#4172799, @efriedma wrote:

I think this is going to change what inputs Sema will accept for "p". If that's intentional, please add test coverage. Otherwise, please make a narrower change.

Your hunch is correct, though this change as written doesn't change what Sema will accept. Testing this locally, I previously had tried just adding Info.setAllowsMemory(); for "p" and NOT Info.setAllowsRegister();; that caused Sema to reject non-ptr arguments.

I will add test cases regardless.

[nickdesaulniers]

• add sema test to validate 'p' inputs as per @efriedma

[nickdesaulniers]

clang/lib/Basic/TargetInfo.cpp

747–748

I wonder if we should make this change for outputs, too?

[nickdesaulniers]

clang/lib/Basic/TargetInfo.cpp

747–748

1. https://reviews.llvm.org/D145429
2. https://reviews.llvm.org/D145430

[nickdesaulniers]

clang/test/Sema/inline-asm-validate.c

9

I should add a test case for "p"(&t).

[nickdesaulniers]

• add test additional case for Sema

[jyknight]

It looks to me from GCC that constraint 'p' is intended to be used internally, not for inline-asm. I question whether the kernel should be using it, and whether we should even implement it at all.

[nickdesaulniers]

In D145416#4173250, @jyknight wrote:

It looks to me from GCC that constraint 'p' is intended to be used internally, not for inline-asm. I question whether the kernel should be using it, and whether we should even implement it at all.

p is a documented "simple constraint": https://gcc.gnu.org/onlinedocs/gcc/Simple-Constraints.html#Simple-Constraints

[jyknight]

‘p’ in the constraint must be accompanied by address_operand as the predicate in the match_operand. This predicate interprets the mode specified in the match_operand as the mode of the memory reference for which the address would be valid.

How do you do that with inline-asm?

[nickdesaulniers]

In D145416#4173258, @jyknight wrote:

‘p’ in the constraint must be accompanied by address_operand as the predicate in the match_operand. This predicate interprets the mode specified in the match_operand as the mode of the memory reference for which the address would be valid.

How do you do that with inline-asm?

Good question. The only tests I see GCC using with "=p" or "p" (no "+p" AFAICT) are:

1. https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/pr58805.c;h=b46f7764260b29bc82fc8226d20019f05c5e05c0;hb=HEAD
2. https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/asm-4.c;h=180578e64e1dd68c2f5619f31fa46ac3433fd226;hb=HEAD

(maybe my grep-fu is weak). These didn't help explain what that sentence means (though maybe my comprehension skills are weak). AFAICT, the docs were added in this massive commit:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=03dda8e3c3b2f

Looking at why the kernel ever used it, it looks like:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.3-rc1&id=97b67ae559947f1e208439a1bf6a734da3087006 introduced it. That commit refers to an issue that was fixed in gcc 4.9:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63637

The linux kernel only support gcc 5.1+, so that issue is now irrelevant from the kernel's standpoint. Maybe it is time to unwind that change (97b67ae559947f1e208439a1bf6a734da3087006)...

[nickdesaulniers]

In D145416#4173368, @nickdesaulniers wrote:

Looking at why the kernel ever used it, it looks like:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.3-rc1&id=97b67ae559947f1e208439a1bf6a734da3087006 introduced it.

Wrong commit. Hard to spot, but "p" was being used previously.

[nickdesaulniers]

In D145416#4173385, @nickdesaulniers wrote:

In D145416#4173368, @nickdesaulniers wrote:

Looking at why the kernel ever used it, it looks like:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.3-rc1&id=97b67ae559947f1e208439a1bf6a734da3087006 introduced it.

Wrong commit. Hard to spot, but "p" was being used previously.

It was https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.3-rc1&id=ed8d9adf357ec331603fa1049510399812cea7e5.

[nickdesaulniers]

https://lore.kernel.org/lkml/[e-mail]/ has historical context around the introduction of "p" in the kernel.

[jyknight]

Also, I note the doc says it's useful for for “load address” and “push address” instructions (note, "load address" means e.g. x86 "lea" instruction) -- which should NOT be dependent upon the value stored in the memory. The x86 backend actually uses a "Ts" constraint for *lea<mode> which is defined by define_address_constraint, documented as making a constraint that is basically equivalent to p (just with different limits as to what kinds of addresses are accepted.)

"p" and other define_address_constraints are also used a lot for "prefetch" kinds of instructions, which also should not be memory-value-dependent.

So, at least per that description, and usage within GCC, I think LLVM's previous behavior of allowing it to move past a store to the same address actually seems correct?

[jyknight]

I believe this is abandoned, correct?

clang/lib/Basic/TargetInfo.cpp

case 'r': // general register.
       break;
     case 'm': // memory operand.
     case 'o': // offsetable memory operand.
     case 'V': // non-offsetable memory operand.
     case '<': // autodecrement memory operand.
     case '>': // autoincrement memory operand.
       Info.setAllowsMemory();
       break;
     case 'g': // general register, memory operand or immediate integer.
     case 'X': // any operand.
       Info.setAllowsRegister();
       Info.setAllowsMemory();
       break;
     case ',': // multiple alternative constraint.  Pass it.
       // Handle additional optional '=' or '+' modifiers.
       if (Name[1] == '=' || Name[1] == '+')
         Name++;
       break;
while (*Name) {
     case 'm': // memory operand.
     case 'o': // offsettable memory operand.
     case 'V': // non-offsettable memory operand.
     case '<': // autodecrement memory operand.
     case '>': // autoincrement memory operand.
       Info.setAllowsMemory();
       break;
     case 'g': // general register, memory operand or immediate integer.
+    case 'p': // address operand.
     case 'X': // any operand.
       Info.setAllowsRegister();
       Info.setAllowsMemory();
       break;
     case 'E': // immediate floating point.
     case 'F': // immediate floating point.
-    case 'p': // address operand.
-      break;
     case ',': // multiple alternative constraint.  Ignore comma.
       break;
     case '#': // Ignore as constraint.
       while (Name[1] && Name[1] != ',')
         Name++;
       break;
     case '?': // Disparage slightly code.
     case '!': // Disparage severely.

clang/test/CodeGen/inline-asm-p-constraint.c

 // RUN: %clang_cc1 -emit-llvm %s -o - -triple aarch64-linux-gnu | FileCheck %s
 void foo(void* ptr) {
   asm volatile("prfm pldl1keep, %a0\n" : : "p" (ptr));
   // CHECK:     call void asm sideeffect "prfm pldl1keep, ${0:a}\0A", "r"(ptr %0)
   // CHECK-NOT: call void asm sideeffect "prfm pldl1keep, ${0:a}\0A", "p"(ptr %0)
 }
 
 int *current_task;
 long x;
 void bar (void) {
   asm ("":"=r"(x):"p"(&current_task));
 }
 // CHECK: %0 = call i64 asm "", "=r,r"(ptr @current_task) #[[ATTR:[0-9]+]]
-// FIXME: should be memory(read)
-// CHECK: attributes #[[ATTR]] = { nounwind memory(none) }
+// CHECK: attributes #[[ATTR]] = { nounwind memory(read) }

clang/test/Sema/inline-asm-validate.c

 // RUN: %clang_cc1 -triple arm64-apple-macosx10.8.0 -fsyntax-only -verify %s
 
 unsigned t, r, *p;
 
-int foo (void) {
+void 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}} expected-note {{use constraint modifier "w"}}
-  return 1;
+  // Ensure we accept pointer and non-pointer arguments to "p" constrained
+  // inputs.
+  asm (""::"p"(t), "p"(p), "p"(&t));
 }