This is an archive of the discontinued LLVM Phabricator instance.

[ms-inline-asm][AVX512] Add ability to use k registers in MS inline asm + fix bag with curly braces
ClosedPublic

Authored by myatsina on Mar 1 2016, 6:36 AM.

Details

Summary

Until now curly braces could only be used in MS inline assembly to mark block start/end.
All curly braces were removed completely at a very early stage.
This approach caused bugs like:
"m{o}v eax, ebx" turned into "mov eax, ebx" without any error.

In addition, AVX-512 added special operands (e.g., k registers), which are also surrounded by curly braces that mark them as such.
Now, we need to keep the curly braces and identify at a later stage if they are marking block start/end (if so, ignore them), or surrounding special AVX-512 operands (if so, parse them as such).

This patch fixes the bug described above and enables the use of AVX-512 special operands.

This review is for the llvm part.
The tests and clang part of the review is: http://reviews.llvm.org/D17766

Diff Detail

Repository
rL LLVM

Event Timeline

myatsina updated this revision to Diff 49485.Mar 1 2016, 6:36 AM
myatsina retitled this revision from to [ms-inline-asm][AVX512] Add ability to use k registers in MS inline asm + fix bag with curly braces.
myatsina updated this object.
myatsina added reviewers: rnk, ehsan, mcrosier, delena.
myatsina set the repository for this revision to rL LLVM.
myatsina added a subscriber: llvm-commits.

ping
the clang part of the review was approved, I'm still waiting for approval on the llvm part

ehsan accepted this revision.Mar 3 2016, 9:19 AM
ehsan edited edge metadata.

This looks good to me, but I would prefer someone else to also look over both parts...

This revision is now accepted and ready to land.Mar 3 2016, 9:19 AM
myatsina updated this revision to Diff 49917.Mar 6 2016, 7:20 AM
myatsina edited edge metadata.

Simplified the code of the llvm part of the patch

delena added inline comments.Mar 6 2016, 11:55 AM
lib/MC/MCParser/AsmParser.cpp
4988

Can this code be moved into parseStatement()?

myatsina added inline comments.Mar 7 2016, 12:05 AM
lib/MC/MCParser/AsmParser.cpp
4988

If we this code to parseStatement() then .s files that contain blocks with braces will be legal.
I don't think we want llvm-mc to be able to accept such syntax.

delena added inline comments.Mar 7 2016, 12:43 AM
lib/MC/MCParser/AsmParser.cpp
5015

Let's take it to a separate function and remove LastStatementLoc.

myatsina updated this revision to Diff 49946.Mar 7 2016, 3:44 AM

Applied requested modifications

Applied requested modifications

A test in the clang part of the review was also changed to reflect this changes (review of the clang-part: http://reviews.llvm.org/D17766)

delena added inline comments.Mar 7 2016, 4:05 AM
lib/MC/MCParser/AsmParser.cpp
1825

line alignment

1837

line alignment

4987

remove {}

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

auto isCurlyBrace = []() {

return getLexer().is(AsmToken::LCurly) || getLexer().is(AsmToken::RCurly);

}

2340

if (getLexer().isNot(AsmToken::EndOfStatement) && !isCurlyBrace())

2349

else if (isCurlyBrace())

2350

brace

myatsina marked an inline comment as done.Mar 7 2016, 7:02 AM
myatsina added inline comments.
lib/Target/X86/AsmParser/X86AsmParser.cpp
2307

We do need the logic that checks that we're dealing with intel syntax and parsing inline asm (otherwise we're parsing asm code of an s file).
I prefer this logic as a variable and not a function.

myatsina updated this revision to Diff 49967.Mar 7 2016, 8:09 AM

Fixed formatting

mcrosier resigned from this revision.Mar 7 2016, 8:48 AM
mcrosier removed a reviewer: mcrosier.
delena accepted this revision.Mar 7 2016, 9:27 AM
delena edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.