Bugfix for Replacement of tied operand of inline asm

[xiangzhangllvm]

Summary

The constraint "0" in the following asm did not consider the its relationship with "=y" when try to replace the type of the operands.

asm ("nop" : "=y"(Mu8_1 ) : "0"(Mu8_0 ));

test case:

typedef unsigned char   __attribute__((vector_size(8))) _m64u8;

int main(void){
_m64u8  __attribute__((aligned(16))) Mu8_0, __attribute__((aligned(16))) Mu8_1;
asm ("nop" : "=y"(Mu8_1 ) : "0"(Mu8_0 ));
return 0;
}

[xiangzhangllvm]

Hi dears, Could you please help me merge the patch. Thank you!

[efriedma]

test/Sema/inline-asm-x86-constraint.c

2 ↗

(On Diff #182729)

Test belongs in test/CodeGen. Please use -emit-llvm and FileCheck the IR, instead of checking how the backend handles it. Shouldn't require x86-registered-target (the x86 frontend bits are always compiled in).

Actually, you could probably stick the function into the existing test test/CodeGen/asm-inout.c, which has another similar test.

[xiangzhangllvm]

!!! Hi, dear efriedma, very sorry! I just saw your reply.
line 2093: getTargetHooks().adjustInlineAsmType(... InputConstraint,...) will just deal with the constrain string, and it can't check the TiedOperand in the function.
So, this will make inconsistent adjust for the operand and its tied operand.
The error will not be found in the IR files, it will cause back end error. like:
"error in backend: Unsupported asm: input constraint with a matching output constraint of incompatible type!"

Please refer the adjustInlineAsmType() function, it will call the following function

static llvm::Type* X86AdjustInlineAsmType(CodeGen::CodeGenFunction &CGF,
                                          StringRef Constraint,
                                          llvm::Type* Ty) {
  bool IsMMXCons = llvm::StringSwitch<bool>(Constraint)
                     .Cases("y", "&y", "^Ym", true)
                     .Default(false);
  if (IsMMXCons && Ty->isVectorTy()) {
    if (cast<llvm::VectorType>(Ty)->getBitWidth() != 64) {
      // Invalid MMX constraint
      return nullptr;
    }
    return llvm::Type::getX86_MMXTy(CGF.getLLVMContext());
  }
  // No operation needed
  return Ty;
}

[efriedma]

We expect that tests for clang IR generation should look something like clang/test/CodeGen/asm-inout.c.

Even though a test like that isn't directly testing the overall behavior, we try to separate tests of clang's behavior from tests of LLVM's behavior. It gives better test coverage by ensuring that clang behaves the way we expect, and the LLVM backend behaves the way we expect. This is important because clang isn't the only frontend that uses LLVM code generation. Also, test written like that tend to be easier to read.

If you want to ensure the IR is eventually lowered correctly, you can add a second test to llvm/test/CodeGen/X86/, which essentially take the IR generated by clang, runs it through llc, and checks we get the expected result. Maybe that test is worth adding here; we don't have very much test coverage for inline asm using MMX.

[xiangzhangllvm]

Thank you, efriedma
But but the LLVM and Clang are different projects, I can commit the change at one time.
I 'll update the patch for clang first.

[xiangzhangllvm]

Hi, efriedma
could you help he commit this patch?
Thank you very much!

[xiangzhangllvm]

We found may tests failed about this issue.
I hope It can be committed.
Thank you very much!

[efriedma]

LGTM; I'll merge it tonight or tomorrow.

[xiangzhangllvm]

In D56990#1426977, @efriedma wrote:

LGTM; I'll merge it tonight or tomorrow.

Thank you very much!

lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitAsmStmt(const AsmStmt &S) {
   std::vector<llvm::Type *> ArgTypes;
   std::vector<llvm::Value*> Args;
 
   // 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;
+
   // 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());
 
     OutputConstraint = AddVariableConstraints(OutputConstraint, *OutExpr,
                                               getTarget(), CGM, S,
                                               Info.earlyClobber());
-
+    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.
     if (!Info.allowsMemory() && hasScalarEvaluationKind(OutExpr->getType())) {
       Constraints += "=" + OutputConstraint;
for (unsigned i = 0, e = S.getNumInputs(); i != e; i++) {
     std::string InputConstraint(S.getInputConstraint(i));
     InputConstraint = SimplifyConstraint(InputConstraint.c_str(), getTarget(),
                                          &OutputConstraintInfos);
 
     InputConstraint = AddVariableConstraints(
         InputConstraint, *InputExpr->IgnoreParenNoopCasts(getContext()),
         getTarget(), CGM, S, false /* No EarlyClobber */);
 
+    std::string ReplaceConstraint (InputConstraint);
     llvm::Value *Arg = EmitAsmInput(Info, InputExpr, Constraints);
 
     // If this input argument is tied to a larger output result, extend the
     // input to be the same size as the output.  The LLVM backend wants to see
     // the input and output of a matching constraint be the same size.  Note
     // that GCC does not define what the top bits are here.  We use zext because
     // that is usually cheaper, but LLVM IR should really get an anyext someday.
     if (Info.hasTiedOperand()) {
if (Info.hasTiedOperand()) {
           Arg = Builder.CreateZExt(Arg, OutputTy);
         else if (isa<llvm::PointerType>(OutputTy))
           Arg = Builder.CreateZExt(Arg, IntPtrTy);
         else {
           assert(OutputTy->isFloatingPointTy() && "Unexpected output type");
           Arg = Builder.CreateFPExt(Arg, OutputTy);
         }
       }
+      // Deal with the tied operands' constraint code in adjustInlineAsmType.
+      ReplaceConstraint = OutputConstraints[Output];
     }
     if (llvm::Type* AdjTy =
-              getTargetHooks().adjustInlineAsmType(*this, InputConstraint,
+          getTargetHooks().adjustInlineAsmType(*this, ReplaceConstraint,
                                                    Arg->getType()))
       Arg = Builder.CreateBitCast(Arg, AdjTy);
     else
       CGM.getDiags().Report(S.getAsmLoc(), diag::err_asm_invalid_type_in_input)
           << InputExpr->getType() << InputConstraint;
 
     // Update largest vector width for any vector types.
     if (auto *VT = dyn_cast<llvm::VectorType>(Arg->getType()))

test/CodeGen/asm-inout.c

 // <rdar://problem/10919182> - This should have both inputs be of type x86_mmx.
 // CHECK: @test5
 typedef long long __m64 __attribute__((__vector_size__(8)));
 __m64 test5(__m64 __A, __m64 __B) {
   // CHECK: call x86_mmx asm "pmulhuw $1, $0\0A\09", "=y,y,0,~{dirflag},~{fpsr},~{flags}"(x86_mmx %{{.*}}, x86_mmx %{{.*}})
   asm ("pmulhuw %1, %0\n\t" : "+y" (__A) : "y" (__B));
   return __A;
 }
+
+// CHECK: @test6
+int test6(void) {
+  typedef unsigned char __attribute__((vector_size(8))) _m64u8;
+  _m64u8 __attribute__((aligned(16))) Mu8_0, __attribute__((aligned(16))) Mu8_1;
+  // CHECK: call x86_mmx asm "nop", "=y,0,~{dirflag},~{fpsr},~{flags}"(x86_mmx %1)
+  asm ("nop" : "=y"(Mu8_1 ) : "0"(Mu8_0 ));
+  return 0;
+}