[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

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

clang/docs/LanguageExtensions.rst

err:
       return -1;
   }
 
 It's important to note that outputs are valid only on the "fallthrough" branch.
 Using outputs on an indirect branch may result in undefined behavior. For
 example, in the function above, use of the value assigned to `y` in the `err`
 block is undefined behavior.
 
+When using tied-outputs (ie. outputs that are inputs and outputs, not just
+outputs) with the `+r` constraint, there is a hidden input that's created
+before the label, so numeric references to operands must account for that.
+
+.. code-block:: c++
+
+  int foo(int x) {
+      // %0 and %1 both refer to x
+      // %l2 refers to err
+      asm goto("# %0 %1 %l2" : "+r"(x) : : : err);
+      return x;
+    err:
+      return -1;
+  }
+
+This was changed to match GCC in clang-13; for better portability, symbolic
+references can be used instead of numeric references.
+
+.. code-block:: c++
+
+  int foo(int x) {
+      asm goto("# %[x] %l[err]" : [x]"+r"(x) : : : err);
+      return x;
+    err:
+      return -1;
+  }
+
 Query for this feature with ``__has_extension(gnu_asm_goto_with_outputs)``.
 
 Objective-C Features
 ====================
 
 Related result types
 --------------------
 

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;
+}