Page MenuHomePhabricator

[X86] VEX/EVEX prefix doesn't work for inline assembly.
ClosedPublic

Authored by LiuChen3 on Oct 22 2020, 9:54 PM.

Details

Summary

For now, we lost the encoding information if we using inline assembly.
The encoding for the inline assembly will keep default even if we add
the vex/evex prefix.

Diff Detail

Event Timeline

LiuChen3 created this revision.Oct 22 2020, 9:54 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 22 2020, 9:54 PM
LiuChen3 requested review of this revision.Oct 22 2020, 9:54 PM
pengfei added inline comments.Oct 22 2020, 10:15 PM
clang/test/CodeGen/X86/att-inline-asm-prefix.c
13

Better adding a no prefix one.

llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
57–70

Can we make these = 1U << aligned?

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
352

"\t{vex}\t" ?

pengfei added inline comments.Oct 22 2020, 10:21 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2824

I think it's reasonable if we generate "{vex}" for input "{vex2}"

LiuChen3 added inline comments.Oct 22 2020, 10:35 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2824

GCC will out put {vex2} if the input is {vex2}.

craig.topper added inline comments.Oct 22 2020, 10:37 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3893–3895

Why do we need Force_VEX3Encoding and IP_USE_VEX3?

llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
67

Why don't these start with IP_?

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
354

Is it important that we use {vex2} instead of just treating it as {vex}?

craig.topper added inline comments.Oct 22 2020, 10:43 PM
llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
354

I guess we'd actually have to print {vex2} instead of {vex} for compatibility with older versions of GNU assember that don't support {vex}. So I guess maybe they should be different as much as I hate wasting flags

craig.topper added inline comments.Oct 22 2020, 10:48 PM
llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
358

We also need to print {disp8} and {disp32} here to fix the same bug with those right?

Does this bug only effect the printing of inline assembly to a .s file? The encoder should work correctly even without this I think?

LiuChen3 added inline comments.Oct 22 2020, 11:12 PM
clang/test/CodeGen/X86/att-inline-asm-prefix.c
15

Does this bug only effect the printing of inline assembly to a .s file?

Yes. Using "-c" to out .o file directly will get right encoding.

But if we firstly output the .s file and then compile it, the end encoding is wrong. For this example, it will all be two-byte vex prefix.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3893–3895

I think this will make all of IP_USE_VEX3 the 3-byte vex prefix instruction output with {vex3}. The IP_USE_VEX3 is for encoder.

craig.topper added inline comments.Oct 22 2020, 11:15 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3893–3895

Isn't this the only place we set IP_USE_VEX3?

LiuChen3 added inline comments.Oct 22 2020, 11:21 PM
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
3893–3895

Yes. I think it's reasonable to use this flag only. And naming other Force_* to IP_USE_* .

LiuChen3 updated this revision to Diff 300188.Oct 23 2020, 1:12 AM

Address comments

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
352

No "\t" needed to add to the end of prefix. The printer will handle it correctly.

This revision is now accepted and ready to land.Oct 24 2020, 10:30 PM
This revision was landed with ongoing or failed builds.Oct 25 2020, 6:09 PM
This revision was automatically updated to reflect the committed changes.

This change probably requires the X86 target.
// REQUIRES: x86-registered-target

Builds which target AArch64 only have been failing due to this change.
http://lab.llvm.org:8011/#/builders/32/builds/291

I have added a fix to run the test only when the X86 target is available. Please feel free to change if it is not the correct fix.
https://github.com/llvm/llvm-project/commit/c551ba0e90bd2b49ef501d591f8362ba44e5484d

I have added a fix to run the test only when the X86 target is available. Please feel free to change if it is not the correct fix.
https://github.com/llvm/llvm-project/commit/c551ba0e90bd2b49ef501d591f8362ba44e5484d

Thanks for your help!