Page MenuHomePhabricator

Bugfix for Replacement of tied operand of inline asm
ClosedPublic

Authored by xiangzhangllvm on Jan 20 2019, 5:21 PM.

Details

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

Diff Detail

Repository
rC Clang

Event Timeline

xiangzhangllvm created this revision.Jan 20 2019, 5:21 PM

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

efriedma added inline comments.Feb 21 2019, 5:46 PM
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.

!!! 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;
}
xiangzhangllvm edited the summary of this revision. (Show Details)Thu, Mar 7, 10:12 PM

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.

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.

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

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

efriedma accepted this revision.Tue, Mar 12, 4:36 PM

LGTM; I'll merge it tonight or tomorrow.

This revision is now accepted and ready to land.Tue, Mar 12, 4:36 PM

LGTM; I'll merge it tonight or tomorrow.

Thank you very much!

This revision was automatically updated to reflect the committed changes.