[clang] Fix handling of physical registers in inline assembly operands.

[jonpa]

Summary

Improve EmitAsmStmt() to

• Not tie physregs with the "+r" constraint, but instead add the hard register as an input constraint. This makes "+r" and "=r":"r" look the same in the output.

Background: Macro intensive code may contain inline assembly statements with multiple operands constrained to the same physreg. Such a case with the operand constraints "+r" : "r" currently triggers the TwoAddressInstructionPass assertion against any extra use of a tied register. Furthermore, TwoAddress will insert a COPY to that physreg even though isel has already done so (for the non-tied use), which may lead to a second redundant instruction currently.

A simple fix for this is to not emit tied physreg uses in the first place for the "+r" constraint, which is what this patch does.

• Give an error on multiple outputs to the same physical register.

This should be reported and this is also what GCC does.

[jonpa]

ping!

[jonpa]

ping!

[jonpa]

Ping!

Last chance for anyone to speak up about any objections to committing this...

[craig.topper]

clang/lib/CodeGen/CGStmt.cpp

2214

Can we get rid of the count call above and just use the bool from the std::pair returned by insert?

[jonpa]

Updated per review by eliminating the call to 'count'.

clang/lib/CodeGen/CGStmt.cpp

2214

Ah, yes.

[jyu2]

clang/lib/CodeGen/CGStmt.cpp

2184

Use llvm::SmallSet?

[jonpa]

Updated per review to use SmallSet.

[aaron.ballman]

clang/lib/CodeGen/CGStmt.cpp

2212

Diagnostics in Clang are typically not grammatically correct, so I think it should be multiple instead of Multiple.

[jonpa]

Updated message string per review.

[aaron.ballman]

This LGTM, but I'm not super well-versed with asm statements to begin with. You should wait a bit for other reviewers to weigh in before landing.

[jyu2]

LGTM

[nickdesaulniers]

https://bugs.llvm.org/show_bug.cgi?id=47837

[jonpa]

Patch temporarily reverted.

[jonpa]

The problem seems to be with a tied earlyclobber operand:

asm("" : "+&r"(a));

Per the comment in InlineAsm::ConstraintInfo::Parse(), only output can be earlyclobber.

I am not sure if the earlyclobber ('&') should with this patch be removed from the added use of the register, or if this has to for some reason really be tied to the def?

[uweigand]

In D87279#2334510, @jonpa wrote:

The problem seems to be with a tied earlyclobber operand:

asm("" : "+&r"(a));

Per the comment in InlineAsm::ConstraintInfo::Parse(), only output can be earlyclobber.

I am not sure if the earlyclobber ('&') should with this patch be removed from the added use of the register, or if this has to for some reason really be tied to the def?

So we have a read/write tied earlyclobber operand that is also tied to a phyreg via an asm register declaration? That means that: the physreg is used as input, it is also used as output, and it is written to before all inputs are read (i.e. no *other* input may share the same register, even if it would otherwise hold the same value).

I believe there is no way to represent the effect of a read/write tied earlyclobber in any other way. E.g. trying to use separate inputs/outputs (one with "=&r", one with "r") does not work since the earlyclobber on the output would prevent the same register to be used for the input, but it *has* to be the same register ...

On on other hand, in this special case the original problem cannot actually occur, because there cannot be any other input using the same physreg (that's what the earlyclobber ensures), so maybe be just don't do that transformation if an earlyclobber is present?

[jonpa]

That makes sense... Patch updated to keep the tying of operands for this case of earlyclobber.

@nickdesaulniers : Can you see if this patch now passes your builds?

[nickdesaulniers]

Thanks for updating the patch.

Please add a test case for the earlyclobber case. In particular, if none of the in-tree tests failed for the initial version of this patch that landed, you should add one that would have.

I should be able to git checkout c78da037783bda0f27f4d82060149166e6f0c796, observe the test case fail, then arc patch D87279 and observe the test case pass.

The current version of this patch, Diff 298624, passes the previously failing Linux kernel builds.

[jonpa]

Sorry, here's the test case included as well...

[nickdesaulniers]

Looks great, thanks!

[nickdesaulniers]

@nathanchance reports in https://github.com/ClangBuiltLinux/linux/issues/1512
that

// clang x.c
register long current_stack_pointer asm("rsp");
void vmcs_set_bits() {
  asm goto("" : "+r"(current_stack_pointer) : : : do_exception);
do_exception:;
}

now seems to be crashing as a result of this change.

[jyknight]

This change seems like it's done in the wrong layer -- why do we add a special-case in Clang to emit "={r11},{r11}", instead of fixing LLVM to not break when given the previously-output "={r11},0"?

Furthermore, since earlyclobber was exempted from this change, doesn't the same TwoAddressInstructionPass bug still affect earlyclobber uses?

[jonpa]

Sorry for the delay.

This change seems like it's done in the wrong layer -- why do we add a special-case in Clang to emit "={r11},{r11}", instead of fixing LLVM to not break when given the previously-output "={r11},0"?

We were starting out with a test case that was the result of macro-intensive code (Linux kernel). It had a tied physreg def/use, with an additional use of the same physreg, which did not compile with clang. There were two problems: TwoAddress could not handle this case and therefore asserted, and also the extra COPY in the output of the same physreg that would also have to be dealt with.

Instead of trying to improve TwoAddress to recognize and handle a special case like this while also making sure no redundant COPYs would be produced, we found it more reasonable to handle this in the front end. The reasoning was that "= {r7},0" should be completely equivalent with "= {r7},{r7}", and they should be treated the same by the compiler. A tied physreg did not seem useful in the first place, so we decided to always emit the second form from the front end.

This is pretty much what the original commit message said: "Background: Macro intensive user code may contain inline assembly statements with multiple operands constrained to the same physreg. Such a case (with the operand constraints "+r" : "r") currently triggers the TwoAddressInstructionPass assertion against any extra use of a tied register. Furthermore, TwoAddress will insert a COPY to that physreg even though isel has already done so (for the non-tied use), which may lead to a second redundant instruction currently. A simple fix for this is to not emit tied physreg uses in the first place for the "+r" constraint, which is what this patch does."

Furthermore, since earlyclobber was exempted from this change, doesn't the same TwoAddressInstructionPass bug still affect earlyclobber uses?

I was never under the impression that this was a bug in TwoAddress, but rather an unhandled ("odd") case of phys-regs. ISel produced the first COPY, and TwoAddress the second one. How should that be cleaned up? Seemed best to avoid adding unnecessarily to TwoAddress if possible to avoid it...

Sorry, we don't have early clobber operands on SystemZ, so I haven't really thought much about that. We decided we had to exclude them from this patch per the reasoning of https://reviews.llvm.org/D87279#2334519.

So I would guess it might still be a problem to compile an early-clobber tied def/use of a physreg with an extra use (or several) of the same physreg. If that case is something you intend to handle, I would agree that perhaps then the front end change of this patch would not be needed anymore as it is the alternative approach.

@nathanchance reports in https://github.com/ClangBuiltLinux/linux/issues/1512 that... now seems to be crashing as a result of this change.

I tested x.c, and it "works" on SystemZ, while it did not pass ISel on X86, from what I understand. Given the above reasoning of the equivalency between the two expressions (use either tied or explicit of the same phys-reg), I would also wonder if X86 is "choking on valid IR"...

As another matter, the SystemZ backend actually can't handle x.c, as it triggers `SpillGPRs.LowGPR != SpillGPRs.HighGPR && "Should be saving %r15 and something else". This is however a problem in the prologe/epilog emission if anything... I guess the case of just clobbering SP in a function has not been seen before.

clang/lib/CodeGen/CGStmt.cpp

 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;

 }
 
 /// AddVariableConstraints - Look at AsmExpr and if it is a variable declared
 /// as using a particular register add that as a constraint that will be used
 /// in this asm stmt.
 static std::string
 AddVariableConstraints(const std::string &Constraint, const Expr &AsmExpr,
                        const TargetInfo &Target, CodeGenModule &CGM,
-                       const AsmStmt &Stmt, const bool EarlyClobber) {
+                       const AsmStmt &Stmt, const bool EarlyClobber,
+                       std::string *GCCReg = nullptr) {
   const DeclRefExpr *AsmDeclRef = dyn_cast<DeclRefExpr>(&AsmExpr);
   if (!AsmDeclRef)
     return Constraint;
   const ValueDecl &Value = *AsmDeclRef->getDecl();
   const VarDecl *Variable = dyn_cast<VarDecl>(&Value);
   if (!Variable)
     return Constraint;
   if (Variable->getStorageClass() != SC_Register)
     return Constraint;
   AsmLabelAttr *Attr = Variable->getAttr<AsmLabelAttr>();
   if (!Attr)
     return Constraint;
   StringRef Register = Attr->getLabel();
   assert(Target.isValidGCCRegisterName(Register));
   // We're using validateOutputConstraint here because we only care if
   // this is a register constraint.
   TargetInfo::ConstraintInfo Info(Constraint, "");
   if (Target.validateOutputConstraint(Info) &&
       !Info.allowsRegister()) {
     CGM.ErrorUnsupported(&Stmt, "__asm__");
     return Constraint;
   }
   // Canonicalize the register here before returning it.
   Register = Target.getNormalizedGCCRegisterName(Register);
+  if (GCCReg != nullptr)
+    *GCCReg = Register.str();
   return (EarlyClobber ? "&{" : "{") + Register.str() + "}";
 }
 
 llvm::Value*
 CodeGenFunction::EmitAsmInputLValue(const TargetInfo::ConstraintInfo &Info,
                                     LValue InputValue, QualType InputType,
                                     std::string &ConstraintStr,
                                     SourceLocation Loc) {
void CodeGenFunction::EmitAsmStmt(const AsmStmt &S) {
   // Keep track of inout constraints.
   std::string InOutConstraints;
   std::vector<llvm::Value*> InOutArgs;
   std::vector<llvm::Type*> InOutArgTypes;
 
   // Keep track of out constraints for tied input operand.
   std::vector<std::string> OutputConstraints;
 
+  // Keep track of defined physregs.
+  llvm::SmallSet<std::string, 8> PhysRegOutputs;
+
   // An inline asm can be marked readonly if it meets the following conditions:
   //  - it doesn't have any sideeffects
   //  - it doesn't clobber memory
   //  - it doesn't return a value by-reference
   // It can be marked readnone if it doesn't have any input memory constraints
   // in addition to meeting the conditions listed above.
   bool ReadOnly = true, ReadNone = true;
 
   for (unsigned i = 0, e = S.getNumOutputs(); i != e; i++) {
     TargetInfo::ConstraintInfo &Info = OutputConstraintInfos[i];
 
     // Simplify the output constraint.
     std::string OutputConstraint(S.getOutputConstraint(i));
     OutputConstraint = SimplifyConstraint(OutputConstraint.c_str() + 1,
                                           getTarget(), &OutputConstraintInfos);
 
     const Expr *OutExpr = S.getOutputExpr(i);
     OutExpr = OutExpr->IgnoreParenNoopCasts(getContext());
 
+    std::string GCCReg;
     OutputConstraint = AddVariableConstraints(OutputConstraint, *OutExpr,
                                               getTarget(), CGM, S,
-                                              Info.earlyClobber());
+                                              Info.earlyClobber(),
+                                              &GCCReg);
+    // Give an error on multiple outputs to same physreg.
+    if (!GCCReg.empty() && !PhysRegOutputs.insert(GCCReg).second)
+      CGM.Error(S.getAsmLoc(), "multiple outputs to hard register: " + GCCReg);
+
     OutputConstraints.push_back(OutputConstraint);
     LValue Dest = EmitLValue(OutExpr);
     if (!Constraints.empty())
       Constraints += ',';
 
     // If this is a register output, then make the inline asm return it
     // by-value.  If this is a memory result, return the value by-reference.
     bool isScalarizableAggregate =
         hasAggregateEvaluationKind(OutExpr->getType());
if (Info.isReadWrite()) {
                                                Arg->getType()))
         Arg = Builder.CreateBitCast(Arg, AdjTy);
 
       // Update largest vector width for any vector types.
       if (auto *VT = dyn_cast<llvm::VectorType>(Arg->getType()))
         LargestVectorWidth =
             std::max((uint64_t)LargestVectorWidth,
                      VT->getPrimitiveSizeInBits().getKnownMinSize());
-      if (Info.allowsRegister())
+      // Only tie earlyclobber physregs.
+      if (Info.allowsRegister() && (GCCReg.empty() || Info.earlyClobber()))
         InOutConstraints += llvm::utostr(i);
       else
         InOutConstraints += OutputConstraint;
 
       InOutArgTypes.push_back(Arg->getType());
       InOutArgs.push_back(Arg);
     }
   }

clang/test/CodeGen/aarch64-inline-asm.c

void test_gcc_registers(void) {
     asm volatile("hvc #0" : : "r" (reg29), "r" (reg30));
     // CHECK: call void asm sideeffect "hvc #0", "{fp},{lr}"
 
     // rN registers when used without register ... asm("rN") syntax
     // should not be remapped.
     asm volatile("mov r0, r1\n");
     // CHECK: call void asm sideeffect "mov r0, r1\0A", ""()
 }
+
+void test_tied_earlyclobber(void) {
+  register int a asm("x1");
+  asm("" : "+&r"(a));
+  // CHECK: call i32 asm "", "=&{x1},0"(i32 %0)
+}

clang/test/CodeGen/systemz-inline-asm-02.c

+// RUN: not %clang_cc1 -triple s390x-linux-gnu -O2 -emit-llvm -o - %s 2>&1 \
+// RUN:  | FileCheck %s
+// REQUIRES: systemz-registered-target
+
+// Test that an error is given if a physreg is defined by multiple operands.
+int test_physreg_defs(void) {
+  register int l __asm__("r7") = 0;
+
+  // CHECK: error: multiple outputs to hard register: r7
+  __asm__("" : "+r"(l), "=r"(l));
+
+  return l;
+}

clang/test/CodeGen/systemz-inline-asm.c

long double test_f128(long double f, long double g) {
   asm("axbr %0, %2" : "=f" (f) : "0" (f), "f" (g));
   return f;
 // CHECK: define void @test_f128(fp128* noalias nocapture sret(fp128) align 8 [[DEST:%.*]], fp128* nocapture readonly %0, fp128* nocapture readonly %1)
 // CHECK: %f = load fp128, fp128* %0
 // CHECK: %g = load fp128, fp128* %1
 // CHECK: [[RESULT:%.*]] = tail call fp128 asm "axbr $0, $2", "=f,0,f"(fp128 %f, fp128 %g)
 // CHECK: store fp128 [[RESULT]], fp128* [[DEST]]
 }
+
+// Test that there are no tied physreg uses. TwoAddress pass cannot deal with them.
+int test_physregs(void) {
+  // CHECK-LABEL: define signext i32 @test_physregs()
+  register int l __asm__("r7") = 0;
+
+  // CHECK: call i32 asm "lr $0, $1", "={r7},{r7}"
+  __asm__("lr %0, %1" : "+r"(l));
+
+  // CHECK: call i32 asm "$0 $1 $2", "={r7},{r7},{r7}"
+  __asm__("%0 %1 %2" : "+r"(l) : "r"(l));
+
+  return l;
+}