Page MenuHomePhabricator

[mlir] X86Vector: Add AVX Rsqrt
ClosedPublic

Authored by cota on Apr 2 2021, 2:54 PM.

Diff Detail

Event Timeline

cota created this revision.Apr 2 2021, 2:54 PM
cota requested review of this revision.Apr 2 2021, 2:54 PM
cota added a subscriber: ezhulenev.Apr 2 2021, 2:55 PM
aartbik added inline comments.Apr 2 2021, 3:56 PM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

So technically you are bringing in AVX into the AVX512 dialect. That is not a big deal, but it makes me wonder, do we want any of the x86_avx512_rsqrtWIDTH versions as well? If so, should we introduce a generic op for this that takes an attribute? (not saying we should since I am not very familiar what you are going to use this for, just thinking out loud here)

275 ↗(On Diff #335032)

Please use the section separating comment used above consistently for new instructions

mlir/test/Dialect/AVX512/legalize-for-llvm.mlir
43 ↗(On Diff #335032)

this omission is pre-existing, but shouldn't we have CHECK-LABEL at the start of each method (or split-input), to make sure CHECKs are leaking between methods in hte long run

mlir/test/Dialect/AVX512/roundtrip.mlir
48 ↗(On Diff #335032)

same question

ezhulenev added inline comments.Apr 5 2021, 11:08 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

Would it be a good idea to create a "generic" AVX dialect, and lower to concrete AVX version based on vector length and availability? Or too much instructions are sufficiently different to make this practical?

ftynse added inline comments.Apr 6 2021, 2:31 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

I find it very surprising to have AVX instructions in the AVX512 dialect, these print as avx512.intr.rsqrt.ps.256 but the instruction in question is _not_ defined in the avx512 set. Maybe we should consider renaming AVX512 to X86 (which would be also consistent with how LLVM has it) and allow any instruction set with several prefixes, i.e. x86.avx512...., x86.avx....

As for the "generic" AVX dialect, why stop at AVX? Can't we just have an operation on a generic vector that lowers to the target-specific intrinsic depending on the target features and vector sizes. This sounds like Vector dialect :)

mlir/test/Dialect/AVX512/roundtrip.mlir
52 ↗(On Diff #335032)

Please add a newline

cota updated this revision to Diff 335620.Apr 6 2021, 12:10 PM
cota marked 4 inline comments as done.

Address some reviewer comments

mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

Maybe we should consider renaming AVX512 to X86 (which would be also consistent with how LLVM has it) and allow any instruction set with several prefixes, i.e. x86.avx512...., x86.avx

This means more work for me, but I have to agree. I'll work on this and send it as a parent patch.

42 ↗(On Diff #335032)

do we want any of the x86_avx512_rsqrtWIDTH versions as well?

I have no use case for them since I am only interested in AVX's rsqrt, but I can probably squeeze this change in.

aartbik added inline comments.Apr 6 2021, 1:59 PM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

Thanks. When given the choice between putting a generic op in vector dialect (viz. VV) or a target specific op (viz. HWV), we should always favor the VV approach, the HWV is a last (and often temporary) solution for experimenting with idiomatic instructions.

As for the new name, X86 makes sense since it keeps a lot of specifics hidden (SSE vz AVX, AVX2, AVX512 etc), but it does not carry the "vector" semantics part ver ystrong, like ARMNeon and ARMSVE do). So AVX would make some sense, or X86SIMD, X86Vector. Not to bikeshed too much, but what do you think?

cota added inline comments.Apr 6 2021, 2:11 PM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

So AVX would make some sense, or X86SIMD, X86Vector.

I like this suggestion -- with just x86 in the name there is a risk this will become more of an an incoherent bag than it needs to be. Plus, it would be confusing since there are other x86-related dialects (e.g. AMX).

My vote is for X86Vector.

ftynse added inline comments.Apr 7 2021, 12:13 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

X86Vector works for me

cota added inline comments.Apr 8 2021, 8:55 AM
mlir/include/mlir/Dialect/AVX512/AVX512.td
42 ↗(On Diff #335032)

I've introduced X86Vector in https://reviews.llvm.org/D100119
I'll update this patch to apply on top of X86Vector once the X86Vector patch is reviewed.

can you please rebase so we can continue the review in the context of the new dialect name?

cota updated this revision to Diff 336966.Apr 12 2021, 2:32 PM

Rebase on X86Vector

cota retitled this revision from [mlir] Add rsqrt to AVX512 dialect to [mlir] X86Vector: Add AVX Rsqrt .Apr 12 2021, 2:33 PM
cota edited the summary of this revision. (Show Details)
cota updated this revision to Diff 336968.Apr 12 2021, 2:39 PM
cota retitled this revision from [mlir] X86Vector: Add AVX Rsqrt to [mlir] X86Vector: Add AVX Rsqrt.

fix newline

aartbik added inline comments.Apr 12 2021, 3:23 PM
mlir/test/Integration/Dialect/Vector/CPU/X86Vector/test-rsqrt.mlir
19

Note that this is old-style (I am responsible for setting the example, but that was in the context of testing the broadcast/insert). But a much better and more readable style for such vector constants is

%v = std.constant dense<[0,125, 0.25 ....]> : vector<8xf32>

In such cases, the CHECKin print also is not needed (which was only added to help the reader see what vectors were constructed.

cota updated this revision to Diff 337021.Apr 12 2021, 6:51 PM
cota marked an inline comment as done.
mlir/test/Integration/Dialect/Vector/CPU/X86Vector/test-rsqrt.mlir
19

Much better! Done.

aartbik accepted this revision.Apr 12 2021, 10:39 PM
This revision is now accepted and ready to land.Apr 12 2021, 10:39 PM
This revision was automatically updated to reflect the committed changes.