[clang] number labels in asm goto strings after tied inputs

[nickdesaulniers]

Summary

I noticed that the following case would compile in Clang but not GCC:

void *x(void) {
  void *p = &&foo;
  asm goto ("# %0\n\t# %l1":"+r"(p):::foo);
  foo:;
  return p;
}

Changing the output template above from %l2 would compile in GCC but not
Clang.

This demonstrates that when using tied outputs (say via the "+r" output
constraint), the hidden inputs occur or are numbered BEFORE the labels,
at least with GCC.

In fact, GCC does denote this in its documentation:
https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Extended-Asm.html#Goto-Labels

Output operand with constraint modifier ‘+’ is counted as two operands
because it is considered as one output and one input operand.

For the sake of compatibility, I think it's worthwhile to just make this
change.

It's better to use symbolic names for compatibility (especially now
between released version of Clang that support asm goto with outputs).
ie. %l1 from the above would be %l[foo]. The GCC docs also make this
recommendation.

Also, I cleaned up some cruft in GCCAsmStmt::getNamedOperand. AFAICT,
NumPlusOperands was no longer used, though I couldn't find which commit
didn't clean that up correctly.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103640
Link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Extended-Asm.html#Goto-Labels

[jyknight]

It's rather sad that GCC made the quite-unintuitive decision to number the arguments in this way -- LONG AFTER clang had already implemented the other way...

[nickdesaulniers]

In D115471#3184384, @jyknight wrote:

It's rather sad that GCC made the quite-unintuitive decision to number the arguments in this way -- LONG AFTER clang had already implemented the other way...

I doubt it was intentional or tested or that it's too late to at least file a bug report.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103640

[apinski-cavium]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096
GCC even documents it this way.

[nickdesaulniers]

• note this semantic change in the release notes, add links to GCC docs

[nickdesaulniers]

• fix typo in release notes

[xbolva00]

clang/docs/ReleaseNotes.rst

157

there is no general documentation? this is kinda bad to have it only in release notes.

[nickdesaulniers]

clang/docs/ReleaseNotes.rst

157

Documenting this in clang/docs/LanguageExtensions.rst would be good.

[nickdesaulniers]

• rebase onto D115688+D115410+D115311, add notes to clang/docs/LanguageExtensions.rst

[void]

clang/docs/LanguageExtensions.rst

1474 ↗

(On Diff #394415)

nit: s/ie./i.e./

clang/docs/ReleaseNotes.rst

- The value of ``__STDC_VERSION__`` has been bumped to ``202000L`` when passing
   ``-std=c2x`` so that it can be distinguished from C17 mode. This value is
   expected to change again when C23 is published.
 - Wide multi-characters literals such as ``L'ab'`` that would previously be interpreted as ``L'b'``
   are now ill-formed in all language modes. The motivation for this change is outlined in
   `P2362 <wg21.link/P2362>`_.
 - Support for ``__attribute__((error("")))`` and
   ``__attribute__((warning("")))`` function attributes have been added.
 - The maximum allowed alignment has been increased from 2^29 to 2^32.
-
 - Clang now supports the ``_BitInt(N)`` family of bit-precise integer types
   from C23. This type was previously exposed as ``_ExtInt(N)``, which is now a
   deprecated alias for ``_BitInt(N)`` (so diagnostics will mention ``_BitInt``
   even if source uses ``_ExtInt``). ``_BitInt(N)`` and ``_ExtInt(N)`` are the
   same types in all respects beyond spelling and the deprecation warning.
   ``_BitInt(N)`` is supported as an extension in older C modes and in all C++
   modes. Note: the ABI for ``_BitInt(N)`` is still in the process of being
   stabilized, so this type should not yet be used in interfaces that require
   ABI stability.
+- When using ``asm goto`` with outputs whose constraint modifier is ``"+"``, we
+  now change the numbering of the labels to occur after hidden tied inputs for
+  better compatibility with GCC.  For better portability between different
+  compilers and versions, symbolic references rather than numbered references
+  should be preferred. See
+  `this thread <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103640>` for more
+  info.
 
 C++ Language Changes in Clang
 -----------------------------
 
 - ...
 
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^

clang/lib/AST/Stmt.cpp

void GCCAsmStmt::setOutputsAndInputsAndClobbers(const ASTContext &C,
   this->Clobbers = new (C) StringLiteral*[NumClobbers];
   std::copy(Clobbers, Clobbers + NumClobbers, this->Clobbers);
 }
 
 /// getNamedOperand - Given a symbolic operand reference like %[foo],
 /// translate this into a numeric value needed to reference the same operand.
 /// This returns -1 if the operand name is invalid.
 int GCCAsmStmt::getNamedOperand(StringRef SymbolicName) const {
-  unsigned NumPlusOperands = 0;
-
   // Check if this is an output operand.
-  for (unsigned i = 0, e = getNumOutputs(); i != e; ++i) {
+  unsigned NumOutputs = getNumOutputs();
+  for (unsigned i = 0; i != NumOutputs; ++i)
     if (getOutputName(i) == SymbolicName)
       return i;
-  }
 
-  for (unsigned i = 0, e = getNumInputs(); i != e; ++i)
+  unsigned NumInputs = getNumInputs();
+  for (unsigned i = 0; i != NumInputs; ++i)
     if (getInputName(i) == SymbolicName)
-      return getNumOutputs() + NumPlusOperands + i;
+      return NumOutputs + i;
 
   for (unsigned i = 0, e = getNumLabels(); i != e; ++i)
     if (getLabelName(i) == SymbolicName)
-      return i + getNumOutputs() + getNumInputs();
+      return NumOutputs + NumInputs + getNumPlusOperands() + i;
 
   // Not found.
   return -1;
 }
 
 /// AnalyzeAsmString - Analyze the asm string of the current asm, decomposing
 /// it into pieces.  If the asm string is erroneous, emit errors and return
 /// true, otherwise return false.

clang/lib/CodeGen/CGStmt.cpp

if (auto *VT = dyn_cast<llvm::VectorType>(Arg->getType()))
           std::max((uint64_t)LargestVectorWidth,
                    VT->getPrimitiveSizeInBits().getKnownMinSize());
 
     ArgTypes.push_back(Arg->getType());
     Args.push_back(Arg);
     Constraints += InputConstraint;
   }
 
+  // Append the "input" part of inout constraints.
+  for (unsigned i = 0, e = InOutArgs.size(); i != e; i++) {
+    ArgTypes.push_back(InOutArgTypes[i]);
+    Args.push_back(InOutArgs[i]);
+  }
+  Constraints += InOutConstraints;
+
   // Labels
   SmallVector<llvm::BasicBlock *, 16> Transfer;
   llvm::BasicBlock *Fallthrough = nullptr;
   bool IsGCCAsmGoto = false;
   if (const auto *GS =  dyn_cast<GCCAsmStmt>(&S)) {
     IsGCCAsmGoto = GS->isAsmGoto();
     if (IsGCCAsmGoto) {
       for (const auto *E : GS->labels()) {
         JumpDest Dest = getJumpDestForLabel(E->getLabel());
         Transfer.push_back(Dest.getBlock());
         llvm::BlockAddress *BA =
             llvm::BlockAddress::get(CurFn, Dest.getBlock());
         Args.push_back(BA);
         ArgTypes.push_back(BA->getType());
         if (!Constraints.empty())
           Constraints += ',';
         Constraints += 'i';
       }
       Fallthrough = createBasicBlock("asm.fallthrough");
     }
   }
 
-  // Append the "input" part of inout constraints last.
-  for (unsigned i = 0, e = InOutArgs.size(); i != e; i++) {
-    ArgTypes.push_back(InOutArgTypes[i]);
-    Args.push_back(InOutArgs[i]);
-  }
-  Constraints += InOutConstraints;
-
   bool HasUnwindClobber = false;
 
   // Clobbers
   for (unsigned i = 0, e = S.getNumClobbers(); i != e; i++) {
     StringRef Clobber = S.getClobber(i);
 
     if (Clobber == "memory")
       ReadOnly = ReadNone = false;

clang/test/CodeGen/asm-goto.c

 loop:
   return 0;
 label_true:
   return 1;
 }
 
 int test4(int out1, int out2) {
   // CHECK-LABEL: define{{.*}} i32 @test4(
-  // CHECK: callbr { i32, i32 } asm sideeffect "jne ${3:l}", "={si},={di},r,i,i,0,1
+  // CHECK: callbr { i32, i32 } asm sideeffect "jne ${5:l}", "={si},={di},r,0,1,i,i
   // CHECK: to label %asm.fallthrough [label %label_true, label %loop]
   // CHECK-LABEL: asm.fallthrough:
   if (out1 < out2)
-    asm volatile goto("jne %l3" : "+S"(out1), "+D"(out2) : "r"(out1) :: label_true, loop);
+    asm volatile goto("jne %l5" : "+S"(out1), "+D"(out2) : "r"(out1) :: label_true, loop);
   else
-    asm volatile goto("jne %l5" : "+S"(out1), "+D"(out2) : "r"(out1), "r"(out2) :: label_true, loop);
-  // CHECK: callbr { i32, i32 } asm sideeffect "jne ${5:l}", "={si},={di},r,r,i,i,0,1
+    asm volatile goto("jne %l7" : "+S"(out1), "+D"(out2) : "r"(out1), "r"(out2) :: label_true, loop);
+  // CHECK: callbr { i32, i32 } asm sideeffect "jne ${7:l}", "={si},={di},r,r,0,1,i,i
   // CHECK: to label %asm.fallthrough2 [label %label_true, label %loop]
   // CHECK-LABEL: asm.fallthrough2:
   return out1 + out2;
 loop:
   return -1;
 label_true:
   return -2;
 }
 
 int test5(int addr, int size, int limit) {
   // CHECK-LABEL: define{{.*}} i32 @test5(
-  // CHECK: callbr i32 asm "add $1,$0 ; jc ${3:l} ; cmp $2,$0 ; ja ${3:l} ; ", "=r,imr,imr,i,0
+  // CHECK: callbr i32 asm "add $1,$0 ; jc ${4:l} ; cmp $2,$0 ; ja ${4:l} ; ", "=r,imr,imr,0,i
   // CHECK: to label %asm.fallthrough [label %t_err]
   // CHECK-LABEL: asm.fallthrough:
   asm goto(
       "add %1,%0 ; "
       "jc %l[t_err] ; "
       "cmp %2,%0 ; "
       "ja %l[t_err] ; "
       : "+r" (addr)
       : "g" (size), "g" (limit)
       : : t_err);
   return 0;
 t_err:
   return 1;
 }
 
 int test6(int out1) {
   // CHECK-LABEL: define{{.*}} i32 @test6(
-  // CHECK: callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${2:l}", "={si},r,i,i,0,{{.*}} i8* blockaddress(@test6, %label_true), i8* blockaddress(@test6, %landing)
+  // CHECK: callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${3:l}", "={si},r,0,i,i,{{.*}} i8* blockaddress(@test6, %label_true), i8* blockaddress(@test6, %landing)
   // CHECK: to label %asm.fallthrough [label %label_true, label %landing]
   // CHECK-LABEL: asm.fallthrough:
   // CHECK-LABEL: landing:
   int out2 = 42;
-  asm volatile goto("testl %0, %0; testl %1, %1; jne %l2" : "+S"(out2) : "r"(out1) :: label_true, landing);
+  asm volatile goto("testl %0, %0; testl %1, %1; jne %l3" : "+S"(out2) : "r"(out1) :: label_true, landing);
 landing:
   return out1 + out2;
 label_true:
   return -2;
 }
+
+// test7 - For the output templates in the asm string (%0, %l2), GCC places
+// hidden inputs tied to outputs ("+r" constraint) BEFORE labels. Test that foo
+// is $2 (or rather ${2:l} because of the l output template) in the emitted asm
+// string, not $1.
+void *test7(void) {
+  // CHECK-LABEL: define{{.*}} i8* @test7(
+  // CHECK: %1 = callbr i8* asm "# $0\0A\09# ${2:l}", "=r,0,i,~{dirflag},~{fpsr},~{flags}"(i8* %0, i8* blockaddress(@test7, %foo))
+  // CHECK-NEXT: to label %asm.fallthrough [label %foo]
+  void *p = &&foo;
+  asm goto ("# %0\n\t# %l2":"+r"(p):::foo);
+foo:
+  return p;
+}
+
+// test8 - the same as test7, but this time we use symbolic names rather than
+// numbered outputs.
+void *test8(void) {
+  // CHECK-LABEL: define{{.*}} i8* @test8(
+  // CHECK: %1 = callbr i8* asm "# $0\0A\09# ${2:l}", "=r,0,i,~{dirflag},~{fpsr},~{flags}"(i8* %0, i8* blockaddress(@test8, %foo))
+  // CHECK-NEXT: to label %asm.fallthrough [label %foo]
+  void *p = &&foo;
+  asm goto ("# %0\n\t# %l[foo]":"+r"(p):::foo);
+foo:
+  return p;
+}