Page MenuHomePhabricator

[mlir][ArmNeon][RFC] Add an ArmNeon dialect
ClosedPublic

Authored by nicolasvasilache on Nov 26 2020, 3:59 AM.

Details

Summary

This revision starts an Arm-specific ArmNeon dialect discussed in the discourse RFC thread.

Diff Detail

Event Timeline

Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested review of this revision.Nov 26 2020, 3:59 AM

What thread was this discussed in?

Update commit message.

rriddle added inline comments.Nov 26 2020, 4:24 AM
mlir/include/mlir/Dialect/Neon/Neon.td
41 ↗(On Diff #307820)

Please resolve these TODOs in this commit.

mlir/lib/IR/StandardTypes.cpp
303 ↗(On Diff #307820)

Use isa here instead.

nicolasvasilache edited the summary of this revision. (Show Details)Nov 26 2020, 4:25 AM
aartbik added inline comments.Dec 2 2020, 1:02 PM
mlir/lib/Conversion/NeonToLLVM/ConvertNeonToLLVM.cpp
112 ↗(On Diff #307828)

This (and other parts) is directly taken from the AVX512 pass. For most part, that is okay with me, but this actually lowering part in AVX512 reflected in these lines of code did not compose very well with the regular vector dialect lowering (which has grown over time with e.g. contract lowering and friends).

Perhaps we can make this part a bit more forward looking on how we want this to run.

mlir/lib/Target/LLVMIR/LLVMNeonIntr.cpp
10 ↗(On Diff #307828)

AVX -> Neon ;-)

aartbik added inline comments.Dec 3 2020, 9:29 AM
mlir/lib/Conversion/NeonToLLVM/ConvertNeonToLLVM.cpp
112 ↗(On Diff #307828)

As discussed in the syn meeting, let's not make this a full pass (and also remove the AVX512 pass), but simply keep this as "populate" methods that are pulled in on demand by the regular vector dialect pass. That will avoid copy-and-pasting the logic and avoid inconsistencies over time. The tests can have their own driver, they don't need a full pass either.

As agreed, I will do the AVX512 pass removal in the meanwhile to set the right example.

AVX512 restructuring landed as https://reviews.llvm.org/D92614
Let's follow that structure for all future architectural-dependent vector dialects....

nicolasvasilache marked 5 inline comments as done.Dec 7 2020, 6:05 AM

Address and rebase

Update commit message

nicolasvasilache retitled this revision from [mlir][Neon][RFC] Add a Neon dialect to [mlir][ArmNeon][RFC] Add an ArmNeon dialect.Dec 7 2020, 6:12 AM
nicolasvasilache edited the summary of this revision. (Show Details)
aartbik accepted this revision.Dec 7 2020, 9:50 AM
aartbik added inline comments.
mlir/include/mlir/Conversion/ArmNeonToLLVM/ArmNeonToLLVM.h
10

I see a lint warning on this line. Not sure if it is useful?

mlir/include/mlir/Conversion/Passes.td
404–405

yes, this is a good point, we should only pull in what is needed

mlir/include/mlir/Dialect/ArmNeon/ArmNeonDialect.h
3

this line is way too long now

This revision is now accepted and ready to land.Dec 7 2020, 9:50 AM
mehdi_amini added inline comments.Dec 7 2020, 1:10 PM
mlir/include/mlir/Conversion/Passes.td
407

Just don't add it here and implement the override method manually? That way you can check for the options.

ftynse added inline comments.Dec 9 2020, 2:03 AM
mlir/lib/Conversion/ArmNeonToLLVM/ArmNeonToLLVM.cpp
25

Could you specify what are the specific adaptations?

70

I don't see "different possible target ops" in the code below. Maybe you could rather take/expose OneToOneLLVMConversionPattern?

nicolasvasilache marked 6 inline comments as done.

Address and rebase

mlir/lib/Conversion/ArmNeonToLLVM/ArmNeonToLLVM.cpp
25

just old crap, dropped, thanks!

70

thanks, much better, paid some of that AVX512 debt too.

Fixes post rebase.

ftynse accepted this revision.Dec 11 2020, 5:42 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMArmNeon.td
2

This looks too long

mlir/lib/Dialect/LLVMIR/IR/LLVMArmNeonDialect.cpp
3

Overflow comment

nicolasvasilache marked an inline comment as done.

Line length.

nicolasvasilache marked an inline comment as done.Dec 11 2020, 5:50 AM
This revision was landed with ongoing or failed builds.Dec 11 2020, 5:51 AM
This revision was automatically updated to reflect the committed changes.
silvas added a subscriber: silvas.Dec 14 2020, 3:19 PM

Did we actually get consensus around the direction here? I didn't see a clear conclusion.