[x86][MC] Fix movdir64b addressing

[akshaykhadse]

Summary

This patch is to fix the issue 63045.

Look at the following code:

int main(int argc, char *argv[]) {
    int arr[1000];
    __asm movdir64b rax, ZMMWORD PTR [arr]
    return 0;
}

Compiling this code using clang -O0 -fasm-blocks bug.c gives the a linker error.

The problem seems to be in the generated assembly. Following is the out put of clang -S -O0 -fasm-blocks bug.c:

movq %rsi, -16(%rbp)
#APP

movdir64b arr, %rax

#NO_APP
xorl %eax, %eax

The symbol arr should be replaced with some address like -4(%rbp).

This makes me believe that issue is not in the linker, but with the ASM parser.

This issue originates with patch D145893. And that's why reverting it fixes the issue. More specifically, the function isMem512_GR64() within the llvm/lib/Target/X86/AsmParser/X86Operand.h file.

[craig.topper]

Test case?

llvm/lib/Target/X86/AsmParser/X86Operand.h

386

How does detecting X86::AH fix the issue?

[akshaykhadse]

I am working on adding a proper test for this issue, but wanted to get a discussion started as I don't believe this is a proper fix. The reason why if (getMemBaseReg() == X86::AH) works is that an operand like ZMMWORD PTR [RAX] has Memory: ModeSize=64,Size=512,BaseReg=ah,Scale=1,Disp=arr i.e. the base reg is ah. Please guide me to indentify such operands when inside the isMem512_GR64() function.

[skan]

Maybe you need to add a test case and explain why we check AH here?

[akshaykhadse]

I am unable to write a .s or .ll test for this issue. The test has to be a end-to-end test, but I don't know how. Basically, __asm movdir64b rax, ZMMWORD PTR [arr] written in a .c file gets incorrectly translated to call void asm sideeffect inteldialect "movdir64b rax, ZMMWORD PTR arr", "~{dirflag},~{fpsr},~{flags}"() #1, !srcloc !6 in the IR. The correct reanslation should be call void asm sideeffect inteldialect "movdir64b rax, ZMMWORD PTR $0", "*m,~{flags},~{dirflag},~{fpsr},~{flags}"(ptr elementtype([1000 x i32]) %arr) #1, !srcloc !6

[skan]

In D151863#4386382, @akshaykhadse wrote:

I am unable to write a .s or .ll test for this issue. The test has to be a end-to-end test, but I don't know how. Basically, __asm movdir64b rax, ZMMWORD PTR [arr] written in a .c file gets incorrectly translated to call void asm sideeffect inteldialect "movdir64b rax, ZMMWORD PTR arr", "~{dirflag},~{fpsr},~{flags}"() #1, !srcloc !6 in the IR. The correct reanslation should be call void asm sideeffect inteldialect "movdir64b rax, ZMMWORD PTR $0", "*m,~{flags},~{dirflag},~{fpsr},~{flags}"(ptr elementtype([1000 x i32]) %arr) #1, !srcloc !6

You can write a CFE test as you described (C->IR)and then add a codegen test(IR -> ASM). Not have to be end-to-end.

[akshaykhadse]

Wait, I think I have a better example to demonstrate what's going on. Please refer this link.

__asm enqcmds rax, [arr] gets translated to enqcmds rax, zmmword ptr [rbp - 4016]
But, __asm movdir64b rax, [arr] gets translated to movdir64b rax, zmmword ptr [arr]
This means that the arr is never lowered to rbp - 4016.

[craig.topper]

MS inline assembly is parsed twice, once by clang and once by the backend.

The issue starts in the clang parsing.

I think the AH comes from this line near the end of X86AsmParser::CreateMemForMSInlineAsm

// Otherwise, we set the base register to a non-zero value
// if we don't know the actual value at this time.  This is necessary to
// get the matching correct in some cases.

BaseReg = BaseReg ? BaseReg : 1;

1 happens to be the value for AH. This register is a GR64 so it fails the isMem512_GR64 check. This causes the assembly to be considered invalid.

Not sure exactly why it picks a non-zero value. I wonder if we should pick a register other than 1 based on 64bit/32bit/16bit mode?

[akshaykhadse]

In D151863#4386528, @craig.topper wrote:

MS inline assembly is parsed twice, once by clang and once by the backend.

The issue starts in the clang parsing.

I think the AH comes from this line near the end of X86AsmParser::CreateMemForMSInlineAsm

// Otherwise, we set the base register to a non-zero value
// if we don't know the actual value at this time.  This is necessary to
// get the matching correct in some cases.

BaseReg = BaseReg ? BaseReg : 1;

1 happens to be the value for AH. This register is a GR64 so it fails the isMem512_GR64 check. This causes the assembly to be considered invalid.

Not sure exactly why it picks a non-zero value. I wonder if we should pick a register other than 1 based on 64bit/32bit/16bit mode?

You are correct. I think it makes sense to pick a value that does not clash with any of the register enum members, but I am not sure if we should have different values for 64bit/32bit/16bit mode.

On a separate note, I wonder if its better to rewrite the check as getMemDisp()->getKind() == llvm::MCExpr::SymbolRef. This way we don't have to check for AH specifically, we just verify if operand is coming from a symbol like arr.

[akshaykhadse]

Add CFE tests

[akshaykhadse]

Please let me know if these tests and changes look okay.

[XinWang10]

clang/test/CodeGen/ms-inline-asm-64.c

79 ↗

(On Diff #527345)

This could be movdir64b?

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86Operand.h

386

Wouldn't this allow "symbol + %eax" to pick the 64-bit instruction which is what the original change was trying to avoid?

[akshaykhadse]

clang/test/CodeGen/ms-inline-asm-64.c

79 ↗

(On Diff #527345)

You are correct. Thanks! I will update it

[akshaykhadse]

Update AsmParser

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1781 ↗

(On Diff #527725)

This sets BaseReg to 0 if its already 0?

llvm/lib/Target/X86/AsmParser/X86Operand.h

386

Now we're not checking the index register if there is no base register?

[akshaykhadse]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1781 ↗

(On Diff #527725)

Oops. Is this even required? I think we can get rid of this. There are no tests that failed.

[akshaykhadse]

llvm/lib/Target/X86/AsmParser/X86Operand.h

386

Let me fix this.

[akshaykhadse]

Address review comments

[akshaykhadse]

If we could get rid of BaseReg = BaseReg ? BaseReg : 1; in X86AsmParser::CreateMemForMSInlineAsm, then we don't need any changes in the X86Operand.h.

I was not able to find any failing tests after making the change. But, this has existed for 11 years in the codebase, so I am not sure if making this change will break things in unexpected ways. Let me know what you think.

[craig.topper]

I still think replacing the 1 with a valid register for the mode is the better fix.

[XinWang10]

This comment was removed by XinWang10.

[akshaykhadse]

In D151863#4390132, @craig.topper wrote:

I still think replacing the 1 with a valid register for the mode is the better fix.

So, should I create a new registers? Something like X86::Sym16, X86::Sym32 and X86::Sym64?

[craig.topper]

In D151863#4390172, @akshaykhadse wrote:

In D151863#4390132, @craig.topper wrote:

I still think replacing the 1 with a valid register for the mode is the better fix.

So, should I create a new registers? Something like X86::Sym16, X86::Sym32 and X86::Sym64?

Use RAX and EAX and AX. It just needs to have the right size.

[akshaykhadse]

Implement solution mentioned in comments

[akshaykhadse]

Fix formatting

[akshaykhadse]

@craig.topper, @skan: Does this look good? or should we remove the following lines entirely from the code

// Otherwise, we set the base register to a non-zero value
// if we don't know the actual value at this time.  This is necessary to
// get the matching correct in some cases.
BaseReg = BaseReg ? BaseReg : 1;

[skan]

Replace the url of the picture in the summary with the plain text?

[skan]

llvm/test/MC/X86/x86-64-movdir64b-intel.s

1 ↗

(On Diff #527797)

The test can pass w/o this patch. I think we should replace it with IR test (llc) here, the input should be the diff in ms-inline-asm-64.c.

[akshaykhadse]

llvm/test/MC/X86/x86-64-movdir64b-intel.s

1 ↗

(On Diff #527797)

It is better to remove this as it does not add any value.

We cannot write a llc test for this because:

• If input IR is call ... "movdir64b eax, ZMMWORD PTR arr ...", the generated assembly will be `movdir64b arr, %rax"
• If input IR is call ... "movdir64b eax, ZMMWORD PTR $0 ...", the generated assembly will be `movdir64b -4016(%rbp), %rax"

In other words, it's responsibility of front-end to generate the correct IR. If the IR is not correct, the back-end will not fix it and incorrect assembly will be generated.

[skan]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1780 ↗

(On Diff #527797)

This logic was firstly added by 7ca135b25ff408fda31f3b01d5e9303054e8267f.

I am not sure whether it's out of date now. But if removing it can make your test pass and not introduce LIT regression, removing should be a better fix.

We shouldn't turn a logic we don't understand into another logic we don't understand, which would confuse later developers

[akshaykhadse]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

1780 ↗

(On Diff #527797)

Ok, let me change this.

[skan]

llvm/test/MC/X86/x86-64-movdir64b-intel.s

1 ↗

(On Diff #527797)

The backend will parse the inline asm in IR too. As I mentioned, you split the end-to-end test to C->IR and IR->AS. So you need to check the inline asm in IR can be parsed correctly.

[akshaykhadse]

Remove logic to set BaseReg to non-zero value

[akshaykhadse]

Add more tests

[MaskRay]

llvm/test/CodeGen/X86/movdir64b-inline-asm-x86_64.ll

1 ↗

(On Diff #528339)

I think we should reuse an existing *-inline-asm-* test file.

See https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer "I don't know an existing test can be enhanced"

[akshaykhadse]

llvm/test/CodeGen/X86/movdir64b-inline-asm-x86_64.ll

1 ↗

(On Diff #528339)

I could not find an appropriate existing test.
I added a couple of cases in the CFE tests clang/test/CodeGen/ms-inline-asm-64.c and clang/test/CodeGen/ms-inline-asm.c.
Now, when I had to add backend tests, I searched for an appropriate file:

llvm-project$ find llvm/test/CodeGen/X86 -type f -name ms-inline-asm-*
llvm/test/CodeGen/X86/ms-inline-asm-variables-x86-2-regs.ll
llvm/test/CodeGen/X86/ms-inline-asm-array.ll
llvm/test/CodeGen/X86/ms-inline-asm-PR44272.ll
llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-2-regs.ll
llvm/test/CodeGen/X86/ms-inline-asm-functions.ll
llvm/test/CodeGen/X86/ms-inline-asm-redundant-clobber.ll
llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-1-reg.ll
llvm/test/CodeGen/X86/ms-inline-asm-variables-x86-1-reg.ll
llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-nopic.ll

As you can see, there are tests for variables, functions, arrays, but no general tests like ms-inline-asm.ll.
My general observation is that for a CFE test *-inline-asm*.c there's an equivalent test in LLVM backend which mentions the original test name in the comment. For example, ms-inline-asm-variables-x86-2-regs.ll mentions that Tests come from "clang/test/CodeGen/ms-inline-asm-variables.c".
However, there is no such file which mentions either ms-inline-asm.c or ms-inline-asm-64.c
So, I picked up an assembly instruction xgetbv from ms-inline-asm.c and searched it in the available .ll backend test via llvm-project$ find llvm/test -type f -name *.ll -exec grep -l "xgetbv" {} \;. There was just one file llvm/test/CodeGen/X86/system-intrinsics-xgetbv.ll that contains test for just this one instruction.
Please suggest an appropriate 32-bit and 64-bit file to add this test

[MaskRay]

llvm/test/CodeGen/X86/movdir64b-inline-asm-x86_64.ll

1 ↗

(On Diff #528339)

Thanks for the investigation. Sometimes we can consider refactoring existing tests before adding a new test.

Even if we add a new file, inline-asm-*.ll may be a better name than *-inline-asm.ll, since the former allows code completion to find related tests more easily.

I'll run away:) My comment is to prompt investigation but I don't plan to look at the problem closely.

[akshaykhadse]

Rename test files

[skan]

LGTM

llvm/lib/Target/X86/AsmParser/X86Operand.h

bool isMem512_RC256X() const {
     return isMem512() && isMemIndexReg(X86::YMM0, X86::YMM31);
   }
   bool isMem512_RC512() const {
     return isMem512() && isMemIndexReg(X86::ZMM0, X86::ZMM31);
   }
   bool isMem512_GR16() const {
     if (!isMem512())
       return false;
+    if (getMemBaseReg() == X86::AH)
+      return true;
     if (getMemBaseReg() &&
         !X86MCRegisterClasses[X86::GR16RegClassID].contains(getMemBaseReg()))
       return false;
     return true;
   }
   bool isMem512_GR32() const {
     if (!isMem512())
       return false;
+    if (getMemBaseReg() == X86::AH)
+      return true;
     if (getMemBaseReg() &&
         !X86MCRegisterClasses[X86::GR32RegClassID].contains(getMemBaseReg()) &&
         getMemBaseReg() != X86::EIP)
       return false;
     if (getMemIndexReg() &&
         !X86MCRegisterClasses[X86::GR32RegClassID].contains(getMemIndexReg()) &&
         getMemIndexReg() != X86::EIZ)
       return false;
     return true;
   }
   bool isMem512_GR64() const {
     if (!isMem512())
       return false;
+    if (getMemBaseReg() == X86::AH)
+      return true;
     if (getMemBaseReg() &&
         !X86MCRegisterClasses[X86::GR64RegClassID].contains(getMemBaseReg()) &&
         getMemBaseReg() != X86::RIP)
       return false;
     if (getMemIndexReg() &&
         !X86MCRegisterClasses[X86::GR64RegClassID].contains(getMemIndexReg()) &&
         getMemIndexReg() != X86::RIZ)
       return false;