This is an archive of the discontinued LLVM Phabricator instance.

[x86][inline-asm][avx512] allow swapping of '{k<num>}' & '{z}' marks
ClosedPublic

Authored by coby on Sep 28 2016, 4:00 AM.

Details

Summary

Desc:

AVX512 allows dest operand to be followed by an op-mask register specifier ('{k<num>}', which in turn may be followed by a merging/zeroing specifier ('{z}')
Currently, the following forms are allowed:
{k<num>}
{k<num>}{z}

This patch allows the following forms:
{z}{k<num>}

and ignores the next form:
{z}

Justification would be quite simple - GCC

Diff Detail

Event Timeline

coby updated this revision to Diff 72794.Sep 28 2016, 4:00 AM
coby retitled this revision from to [x86][inline-asm][avx512] allow swapping of '{k<num>}' & '{z}' marks.
coby updated this object.
coby added reviewers: delena, myatsina.
coby set the repository for this revision to rL LLVM.
coby updated this object.
coby added a subscriber: llvm-commits.
coby added a reviewer: rnk.Sep 28 2016, 4:50 AM
delena added inline comments.Sep 28 2016, 5:33 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1887 ↗(On Diff #72794)

Please add more comments about functionality.

1888 ↗(On Diff #72794)

I suggest to remove IsExpected. Just look for Z and return true if found.

1908 ↗(On Diff #72794)

Comments about what this function does.

1909 ↗(On Diff #72794)

I suggest to return found/not-found. Looks like you continue parsing after error.

1914 ↗(On Diff #72794)

can you use "auto" instead of std::unique_ptr<X86Operand> ?

1916 ↗(On Diff #72794)

Why return !Error() ??
Should we assume that Error() returns true?

coby updated this revision to Diff 72954.Sep 29 2016, 12:19 AM
coby removed rL LLVM as the repository for this revision.

Simplification

coby updated this revision to Diff 72993.Sep 30 2016, 12:37 AM
coby set the repository for this revision to rL LLVM.
delena accepted this revision.Sep 30 2016, 1:09 AM
delena edited edge metadata.
This revision is now accepted and ready to land.Sep 30 2016, 1:09 AM
rnk added inline comments.Sep 30 2016, 8:27 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1884 ↗(On Diff #72993)

Please use the asmparser the convention that 'true' represents an error condition. I'm aware that it's surprising, but I want the code to be consistent until someone decides to do a global refactoring.

1897–1899 ↗(On Diff #72993)

This can be simplified to:

if (parseToken(AsmToken::RCurly, "expected } at this point"))
  return true;
1908 ↗(On Diff #72794)

Oh dear, this is opposite from the convention of the rest of the asmparser. This has exactly one call site and it is local to this file. Do you mind flipping the convention to match the rest of this file and all other asm parsers?

coby marked 5 inline comments as done.Oct 5 2016, 3:49 AM

@rnk: Then you suggest i'll flip HandleAVX512Opernad as well? As it is not adhering the convention as well

rnk edited edge metadata.Oct 5 2016, 1:47 PM
In D25013#562032, @coby wrote:

@rnk: Then you suggest i'll flip HandleAVX512Opernad as well? As it is not adhering the convention as well

Yes please. It looks like a small change.

coby updated this revision to Diff 73810.Oct 6 2016, 9:46 AM
coby edited edge metadata.

Flipping boolean return value(s) so proposed changes will be on par with current convention.
affected subroutines:

ParseZ
HandleAVX512Operand
rnk accepted this revision.Oct 11 2016, 3:23 PM
rnk edited edge metadata.

looks good, but I'd recommend using the parse* helpers.

lib/Target/X86/AsmParser/X86AsmParser.cpp
1933–1935 ↗(On Diff #73810)

This is also equivalent to parseToken

1897–1899 ↗(On Diff #72993)

Any reason not to do this?

coby added inline comments.Oct 18 2016, 1:40 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1897–1899 ↗(On Diff #72993)

Some.
One would be conformity, the second will apply to readability and the other is a somewhat of a flavor issue:

  1. While 'parseToken' might (or might not, haven't really checked) be considered as a common way of reporting errors - it is no where to be seen on the currently reviewed source file. hence, at least until global re-factorization is at hand - I would tend to stick to the old and (possibly) good.
  2. the title 'parseToken' does brilliant job stating its role as a Parsing subroutine, but fails to reveal its part as an error-reporting mechanism.
  3. imho, and as a side note, the entire error reporting handling is a bit of lacky, mainly the need to dictate an error message each time, but it is a subject for another discussion..
This revision was automatically updated to reflect the committed changes.