This is an archive of the discontinued LLVM Phabricator instance.

[mlir][AVX512] Start a primitive AVX512 dialect
ClosedPublic

Authored by nicolasvasilache on Mar 11 2020, 6:01 AM.

Details

Summary

The Vector Dialect document discusses the vector abstractions that MLIR supports and the various tradeoffs involved.

One of the layer that is missing in OSS atm is the Hardware Vector Ops (HWV) level.

This revision proposes an AVX512-specific to add a new Dialect/Targets/AVX512 Dialect that would directly target AVX512-specific intrinsics.

Atm, we rely too much on LLVM’s peephole optimizer to do a good job from small insertelement/extractelement/shufflevector. In the future, when possible, generic abstractions such as VP intrinsics should be preferred.

This is related to the discussion in: https://llvm.discourse.group/t/rfc-starting-an-avx512-target-specific-dialect-rebooted/688/9

The revision will allow trading off HW-specific vs generic abstractions in MLIR.

Diff Detail

Event Timeline

Just fly by :) It seems you'll need to update the cmake file for mlir-translate as your translate pass is not getting linked in.

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
22–34

Accidental paste?

marbre added a subscriber: marbre.Mar 12 2020, 6:07 AM
nicolasvasilache retitled this revision from [mlir][RFC][AVX512] Start a primitive AVX512 dialect to [mlir][AVX512] Start a primitive AVX512 dialect.
nicolasvasilache edited the summary of this revision. (Show Details)

Update commit message.

Use side effect interfaces.
Remove spurious license.

rriddle added inline comments.Mar 17 2020, 7:51 PM
mlir/include/mlir/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.h
13

Can we trim this include, and replace with forward declare?

25

nit: Prefer std::unique_ptr<> here instead.

mlir/include/mlir/Dialect/AVX512/AVX512.td
39

nit: Can you use let arguments = and let results = for these ops instead of using Arguments/Results? It is much cleaner IMO

63

This should be possible now by using the TypesMatchWith constraint

94

Same here.

mlir/include/mlir/Dialect/AVX512/AVX512Dialect.h
14

This header guard seems wrong.

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
46

This doesn't look like it has been formatted.

84

Can you hoist most of this so that we don't need to spread it across five lines?

106

Same here.

mlir/lib/Target/LLVMIR/AVX512Intr.cpp
30 ↗(On Diff #250968)

Not sure if this is possible yet, but we should really remove the need to use inheritance for extension.

rriddle added inline comments.Mar 17 2020, 8:27 PM
mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
37

Note that a few things have changed at head:

PatternMatchResult -> LogicalResult
matchSuccess -> success
matchFailure -> failure

nicolasvasilache marked 9 inline comments as done.

Address review comments.

nicolasvasilache marked an inline comment as done.Mar 17 2020, 9:08 PM
nicolasvasilache added inline comments.
mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
46

tried both internal and oss git-clang-format, they seem to agree here.

nicolasvasilache marked 5 inline comments as done.

Address comments.

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
37

Rebased.

mlir/lib/Target/LLVMIR/AVX512Intr.cpp
30 ↗(On Diff #250968)

I don't see any CRTP on ModuleTranslation yet, can we keep this as an NFC followup refactor?

nicolasvasilache marked an inline comment as done.Mar 17 2020, 9:19 PM

What's the plan with separating ops taking std types from ops taking LLVM types? According to a comment, you want to do that, maybe do it immediately to avoid file moves and build system updates later.

mlir/include/mlir/Dialect/AVX512/AVX512.td
66

It should be possible to just omit this

126

Nit: can we use consistent formatting? Previous block has 4 leading spaces in exactly the same sitatuion.

mlir/include/mlir/IR/OpImplementation.h
112

Use git-clang-format to avoid spurious reformatting in future commits

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
132

It looks like you could have

template <typename OpTy>
Type getVectorElementType(OpTy op) {
  return op.src().getType().cast<VectorType>().getElementType();
}

that you could reuse in all four patterns as

if (getVectorElementType(cast<OpTy>(op)).isF32()) { ... }
164

Nit: let's turn off clang-format here and just have one pattern per line. It reduces history churn due to reflowing lines every time one adds a new pattern in the list.

186

We can go full metaprogramming and generate this function (and potentially the intrinsic-related ops as well)

mlir/lib/Target/LLVMIR/AVX512Intr.cpp
30 ↗(On Diff #250968)

@rriddle it is not possible yet, but it is on my todo list. I won't get to it in the next couple of weeks, so if somebody else has cycles, please go ahead :)

mlir/tools/mlir-opt/CMakeLists.txt
8 ↗(On Diff #250985)

Leftover debug?

nicolasvasilache marked 11 inline comments as done.Mar 19 2020, 7:58 PM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/OpImplementation.h
112

I have, it is another one of those cases where the config in phab does not agree.
@mehdi_amini and @jpienaar are the most up to date on this.
Forcing manually to what phab wants.

604

same here re format

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
186

Ack

nicolasvasilache marked 2 inline comments as done.Mar 19 2020, 7:58 PM

Address review comments.

Harbormaster completed remote builds in B49827: Diff 251539.
ftynse accepted this revision.Mar 20 2020, 9:56 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/AVX512/AVX512.td
17

Is this still necessary?

45

Nit: I think it lowers to MLIR dialect operations rather than llvm intrinsics

mlir/lib/Conversion/AVX512ToLLVM/ConvertAVX512ToLLVM.cpp
179

I think you can now do addIllegalDialect<AVX512Dialect>

mlir/lib/Target/LLVMIR/LLVMAVX512Intr.cpp
45

I don't see a problem including this into the "main" mlir to llvm translation.

nicolasvasilache marked 6 inline comments as done.Mar 20 2020, 11:10 AM
nicolasvasilache added inline comments.
mlir/lib/Target/LLVMIR/LLVMAVX512Intr.cpp
45

I am not sure how to do that while still controlling opt-in at link time.
Let's iterate on this in-tree if you don't mind.

nicolasvasilache marked an inline comment as done.

Address review comments.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 20 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.
liuz added a subscriber: liuz.Mar 25 2020, 12:00 PM