This is an archive of the discontinued LLVM Phabricator instance.

[x86][MC] Fix movdir64b addressing
ClosedPublic

Authored by akshaykhadse on May 31 2023, 9:27 PM.

Details

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.

Diff Detail

Event Timeline

akshaykhadse created this revision.May 31 2023, 9:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 9:27 PM
akshaykhadse requested review of this revision.May 31 2023, 9:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 9:27 PM
akshaykhadse edited the summary of this revision. (Show Details)May 31 2023, 9:32 PM
akshaykhadse edited the summary of this revision. (Show Details)

Test case?

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

How does detecting X86::AH fix the issue?

akshaykhadse added a comment.EditedMay 31 2023, 9:38 PM

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 added a comment.May 31 2023, 9:39 PM

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

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 added a comment.May 31 2023, 11:58 PM

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 added a comment.EditedJun 1 2023, 12:29 AM

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.

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 added a comment.EditedJun 1 2023, 2:37 AM

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.

Add CFE tests

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 2:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

XinWang10 added inline comments.Jun 1 2023, 6:43 PM
clang/test/CodeGen/ms-inline-asm-64.c
79

This could be movdir64b?

craig.topper added inline comments.Jun 1 2023, 6:43 PM
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 added inline comments.Jun 1 2023, 7:32 PM
clang/test/CodeGen/ms-inline-asm-64.c
79

You are correct. Thanks! I will update it

Update AsmParser

craig.topper added inline comments.Jun 1 2023, 9:43 PM
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 added inline comments.Jun 1 2023, 9:47 PM
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 added inline comments.Jun 1 2023, 10:07 PM
llvm/lib/Target/X86/AsmParser/X86Operand.h
386

Let me fix this.

Address review comments

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.

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

This comment was removed by XinWang10.

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?

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.

Implement solution mentioned in comments

Fix formatting

akshaykhadse added a comment.EditedJun 4 2023, 7:11 PM

@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 added a comment.Jun 4 2023, 10:38 PM

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

skan added inline comments.Jun 4 2023, 10:45 PM
llvm/test/MC/X86/x86-64-movdir64b-intel.s
2

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 added inline comments.Jun 4 2023, 10:57 PM
llvm/test/MC/X86/x86-64-movdir64b-intel.s
2

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 added inline comments.Jun 4 2023, 11:10 PM
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 added inline comments.Jun 4 2023, 11:14 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
1780 ↗(On Diff #527797)

Ok, let me change this.

skan added inline comments.Jun 4 2023, 11:14 PM
llvm/test/MC/X86/x86-64-movdir64b-intel.s
2

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.

Remove logic to set BaseReg to non-zero value

Add more tests

MaskRay added a subscriber: MaskRay.Jun 5 2023, 9:35 PM
MaskRay added inline comments.
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 marked an inline comment as done.Jun 6 2023, 12:23 AM
akshaykhadse added inline comments.
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 added inline comments.Jun 6 2023, 4:15 PM
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.

Rename test files

akshaykhadse edited the summary of this revision. (Show Details)Jun 7 2023, 11:51 PM
akshaykhadse edited the summary of this revision. (Show Details)
skan accepted this revision.Jun 7 2023, 11:59 PM

LGTM

This revision is now accepted and ready to land.Jun 7 2023, 11:59 PM
This revision was automatically updated to reflect the committed changes.