This is an archive of the discontinued LLVM Phabricator instance.

[mlir][AVX512] Add mask.compress to AVX512 dialect.
ClosedPublic

Authored by springerm on Feb 27 2021, 1:27 AM.

Details

Summary

Adds mask.compress to the AVX512 dialect and defines a lowering to the LLVM dialect.

Diff Detail

Event Timeline

springerm created this revision.Feb 27 2021, 1:27 AM
springerm requested review of this revision.Feb 27 2021, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2021, 1:27 AM
springerm added inline comments.Feb 27 2021, 1:33 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
34

There are _mm512_maskz_compress_ps etc. intrinsics for zeroed compress, but they use the same AVX instruction as _mm512_mask_compress_ps: mask.compress. So technically we don't really need the zeroed variant as a separate operation, and can instead set src := vector of all zeros.

Should I keep MaskzCompressOp or remove it from this commit? (In most cases (e.g., sparse dot product), we probably use the zeroed variant.)

aartbik added inline comments.Feb 27 2021, 5:37 PM
mlir/include/mlir/Dialect/AVX512/AVX512.td
36

Maybe say "Register k ...." just so it does not look like a typo to people less familiar with the mask registers

47

put quotes around the names "a" so it does not look like a dangling article

62

same as above

ftynse added inline comments.Mar 1 2021, 5:14 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
34

I don't see the value proposition of the zeroed variant, especially since it lowers to the same LLVM IR intrinsic as the non-zeroed one. If you want to avoid looking up the src operand and checking if it is a constant zero vector, you can either have a unit attribute zero on the "main" op + make the src argument optional + add a verifier that either the operand or the attribute is present or have a constant_src attribute with _any_ value (not only zero), that can be used instead of the src argument + the same scheme as above. The latter is closer in spirit to Linalg, which is the dialect that will target this I presume.

47

Surrounding variable names with quotes or backticks is a generally good practice. Doing so for k in error messages in TypesMatchWith would also address the issue of misreading them as a typo.

mlir/include/mlir/Dialect/LLVMIR/LLVMAVX512.td
36

Naming nit: this is a specific kind of overloading that is fully defined by the first result, there may be other kinds so I wouldn't use IntrOverloadedOp here only to find out later that it cannot be used for, e.g., an op that is overloaded by its second operand.

42

Nit: add a comment for 1 for consistency with other literal arguments.

nicolasvasilache accepted this revision.Mar 1 2021, 5:31 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/AVX512/AVX512.td
34

Hmm I thought I had replied but apparently did not commit ..

I'm be curious to see if using one vs the other can result in significant difference in the final assembly generated by LLVM in interesting cases (likely a few commits down the line until we have a full dot/matvec example). Technically there may be a difference for LLVM's optimizations.
Not sure yet whether this matters in practice but I'd be interested in a confirmation based on data.

Re. where the funneling should go, we could indeed drop 1 op and use an attribute.
However, I think we would then need to make some changes to the autogenerated LLVMIR translation in non-trivial ways (I forget)?
If so, is it worth it?

This revision is now accepted and ready to land.Mar 1 2021, 5:31 AM
springerm updated this revision to Diff 328368.Mar 4 2021, 8:08 PM
springerm marked 7 inline comments as done.

Remove maskz.compress. Make src optional.

springerm added inline comments.Mar 4 2021, 8:09 PM
mlir/include/mlir/Dialect/AVX512/AVX512.td
34

I decided to add a constant_src attribute. If neither constant_src nor src is specified, a vector of all zeros is used (same as maskz.compress).

mlir/include/mlir/Dialect/LLVMIR/LLVMAVX512.td
36

Added a comment. I can change this if necessary when adding additional ops.

springerm edited the summary of this revision. (Show Details)Mar 4 2021, 8:10 PM
This revision was automatically updated to reflect the committed changes.