Page MenuHomePhabricator

[mlir] Rename AVX512 dialect to X86Vector
ClosedPublic

Authored by cota on Apr 8 2021, 8:52 AM.

Details

Summary

We will soon be adding non-AVX512 operations to MLIR, such as AVX's rsqrt. In https://reviews.llvm.org/D99818 several possibilities were discussed, namely to (1) add non-AVX512 ops to the AVX512 dialect, (2) add more dialects (e.g. AVX dialect for AVX rsqrt), and (3) expand the scope of the AVX512 to include these SIMD x86 ops, thereby renaming the dialect to something more accurate such as X86Vector.

Consensus was reached on option (3), which this patch implements.

Diff Detail

Event Timeline

cota created this revision.Apr 8 2021, 8:52 AM
cota requested review of this revision.Apr 8 2021, 8:52 AM

I don't think we have so many clients that renaming isn't possible, do we? We have done that before with much more heavily used files.
It's not a super big deal, but this way we lose the revision history of all copied files.

So unless there is a very strong reason to do it this way (or I am wrong in revision history), can we just move the files?

cota updated this revision to Diff 336299.Apr 8 2021, 7:42 PM
  • [mlir] Rename AVX512 dialect to X86Vector
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 7:42 PM
cota retitled this revision from [mlir] Add X86Vector dialect to supersede the AVX512 dialect to [mlir] Rename AVX512 dialect to X86Vector.Apr 8 2021, 7:43 PM
cota edited the summary of this revision. (Show Details)
cota updated this revision to Diff 336300.Apr 8 2021, 7:56 PM

The previous push used a different diffbase than the original push,
which messed up the diff. Push a diff with the original diffbase now.

bondhugula requested changes to this revision.Apr 8 2021, 9:11 PM
bondhugula added a subscriber: bondhugula.

There isn't a commit summary at all. It'd be good to explain the rationale for the renaming.

This revision now requires changes to proceed.Apr 8 2021, 9:11 PM
cota edited the summary of this revision. (Show Details)Apr 9 2021, 7:00 AM

There isn't a commit summary at all. It'd be good to explain the rationale for the renaming.

Added context for this change, thanks.

ftynse accepted this revision.Apr 9 2021, 7:09 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/X86Vector/X86Vector.td
1

Please fix the comment overflow

mlir/lib/Dialect/X86Vector/Transforms/LegalizeForLLVMExport.cpp
1–2

Please fix the comment overflow

cota updated this revision to Diff 336447.Apr 9 2021, 7:22 AM

Fix overly long comments

cota marked 2 inline comments as done.Apr 9 2021, 7:23 AM
aartbik added inline comments.Apr 9 2021, 11:18 AM
mlir/include/mlir/Dialect/X86Vector/Transforms.h
2

Just curious, why does this file (and the cmake) still show up as delete/add pair

cota marked an inline comment as done.Apr 9 2021, 12:11 PM
cota added inline comments.
mlir/include/mlir/Dialect/X86Vector/Transforms.h
2

That depends on the similarity index that git uses:

$ git diff help
[...]
-M[<n>], --find-renames[=<n>]
           Detect renames. If n is specified, it is a threshold on the similarity index (i.e. amount of addition/deletions compared to the file’s size). For example,
           -M90% means Git should consider a delete/add pair to be a rename if more than 90% of the file hasn’t changed. Without a % sign, the number is to be read as a
           fraction, with a decimal point before it. I.e., -M5 becomes 0.5, and is thus the same as -M50%. Similarly, -M05 is the same as -M5%. To limit detection to
           exact renames, use -M100%. The default similarity index is 50%.

For instance with -M4 this file is detected as a rename. The CMake file has all of its non-whitespace lines changed so it cannot be detected as a rename even with -M1.

aartbik accepted this revision.Apr 9 2021, 12:48 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/X86Vector/Transforms.h
2

Makes sense. Thanks.

We will soon be adding non-AVX512 operations to MLIR, such as AVX's rsqrt. In https://reviews.llvm.org/D99818 several possibilities were discussed, namely to (1) add non-AVX512 ops to
the AVX512 dialect, (2) add more dialects (e.g. AVX dialect for AVX rsqrt), and (3) expand the scope of the AVX512 to include these SIMD x86 ops, thereby renaming the dialect to
something more accurate such as X86Vector.
Consensus was reached on option (3), which this patch implements.

Are you sure about adding non AVX ops to this, i.e., renaming this to X86Vector vs just to AVX? It was a bit confusing without the reference to the rsqrt revision, which would still keep this dialect to AVX. I see that there was a consensus on D99818 to call this X86Vector, but it's always confusing when the renaming is done before any actual non-AVX ops are added or even a plan for that. Is there a proposal for the first non AVX op to be added to this and which one it would be? I also see @ftynse suggested going with AVX there.

Maybe a child diff with the new op?

Is there a proposal for the first non AVX op to be added to this and which one it would be?

There is a patch, D99818, which triggered the discussion about renaming. That patch adds an AVX op. I am highly supportive of renaming the dialect in a separate patch.

I also see @ftynse suggested going with AVX there.

You must be misreading, I did not suggest this. This will be equally confusing to have AVX512 ops in the AVX dialect and AVX ops in the AVX512 dialect; these are two different instruction sets.

Is there a proposal for the first non AVX op to be added to this and which one it would be?

There is a patch, D99818, which triggered the discussion about renaming. That patch adds an AVX op. I am highly supportive of renaming the dialect in a separate patch.

I was asking about a "non AVX op" - as you point out, that's adding an AVX op. (I was aware of D99818 after Aart referred to that above.)

I also see @ftynse suggested going with AVX there.

You must be misreading, I did not suggest this. This will be equally confusing to have AVX512 ops in the AVX dialect and AVX ops in the AVX512 dialect; these are two different

I might have misread the multiply nested context; looks like it was someone else.

cota marked an inline comment as done.Apr 12 2021, 8:53 AM

So far I only have immediate plans of adding avx.rsqrt to the dialect. I'd expect AVX, AVX512 and SSE ops to be added to this dialect in the future.

I think AVX is an OK name although if we end up adding SSE ops then AVX could be confusing.

Furthermore, if we prefix the ops with the dialect's name, which AFAICT seems to be the convention in MLIR, then naming the dialect AVX might be confusing/ugly. For example, we'd have avx.avx.rsqrt and avx.avx512.rsqrt$width; I'd rather have something else in the prefix or just drop the prefix and call those ops avx.rsqrt and avx512.rsqrt.

I don't feel strongly about this, so if you want me to proceed with AVX (I presume by dropping avx as a prefix for all ops) I'll happily do that.

I have expressed by concerns about the dialect AVX here and in the previous discussion. We will have to rename it the moment somebody wants SSE! I don't understand what is blocking here, the renamed dialect intends to contain ops related to X86 vectors. It does not yet, but this should not preclude us from stating the intent and proceeding. If somebody fills super-strongly about the name, I can waste my time to add a random SSE op once its landed...

nicolasvasilache accepted this revision.Apr 12 2021, 9:15 AM

x86vector is fine for now.

As we evolve we may want to get into e.g. x86.avx.xyz and x86.avx512.pqr but we can easily iterate in-tree as we grow the number of use cases.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 12 2021, 10:20 AM
Closed by commit rG8508a63b887e: [mlir] Rename AVX512 dialect to X86Vector (authored by cota, committed by ftynse). · Explain Why
This revision was automatically updated to reflect the committed changes.