Page MenuHomePhabricator

[llvm][mlir] Promote the experimental reduction intrinsics to be first class intrinsics.
ClosedPublic

Authored by aemerson on Sat, Oct 3, 12:27 PM.

Diff Detail

Event Timeline

aemerson created this revision.Sat, Oct 3, 12:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
aemerson requested review of this revision.Sat, Oct 3, 12:27 PM

@aartbik @rriddle @nicolasvasilache MLIR also needed updating to use these new intrinsics, please take a look.

nikic added inline comments.Sat, Oct 3, 1:05 PM
llvm/docs/LangRef.rst
15547

Underlines for titles need adjustment.

llvm/test/Bitcode/upgrade-vecreduce-intrinsics.ll
87

Please also add fast variants for these two tests, to make sure the operand is not replaced with 0/1 if the v2 intrinsics were used.

RKSimon added inline comments.Sat, Oct 3, 1:12 PM
llvm/lib/IR/AutoUpgrade.cpp
3647–3648

Can we not just drop support for the v1 experimental fmul/fadd now? As @nikic said there is a danger of this matching against the v2 variants upgraded above as well.

aemerson added inline comments.Sat, Oct 3, 4:25 PM
llvm/lib/IR/AutoUpgrade.cpp
3647–3648

Yeah I think we can probably drop support. I'll remove this special handling so we're only effectively renaming the existing set.

aemerson updated this revision to Diff 296010.Sat, Oct 3, 4:44 PM
aemerson marked an inline comment as done.

Don't upgrade v1 intrinsics.

ftynse added a comment.Mon, Oct 5, 1:31 AM

@aartbik @rriddle @nicolasvasilache MLIR also needed updating to use these new intrinsics, please take a look.

MLIR part LGTM

Please can you add an entry to the llvm release docs

aemerson updated this revision to Diff 296236.Mon, Oct 5, 11:10 AM

Add a note to the release notes.

nikic added inline comments.Mon, Oct 5, 11:58 AM
llvm/docs/LangRef.rst
15556

There's more underlines that need adjustment here.

llvm/lib/IR/AutoUpgrade.cpp
745

Seeing this, I'm wondering whether we shouldn't change the type signature for fadd/fmul to omit the separate scalar type. It should be sufficient to have vector.reduce.fadd.v4f32 rather than vector.reduce.f32.v4f32. The scalar argument has to match the vector type, just the like return type already has to match.

spatel added inline comments.Mon, Oct 5, 12:56 PM
llvm/docs/LangRef.rst
15597

Given that we're using the term "sequential" in D88791 (and I think that's a good change), should we do the same here for consistency? That could be considered an improvement independently of removing "experimental".

aemerson added inline comments.Mon, Oct 5, 5:35 PM
llvm/docs/LangRef.rst
15597

Sounds like a good idea. I'll do that as a separate change after D88791 lands.

aemerson updated this revision to Diff 296603.Tue, Oct 6, 10:27 PM

Remove the scalar input overload for fadd/fmul. Now the signature is simple llvm.reduce.fadd.v4f32 rather than llvm.reduce.fadd.f32.v4f32

Also fix some langref formatting, and fold in the change to use "sequential" instead of "ordered" while I'm there.

One last minor comment - anyone else?

llvm/test/Instrumentation/MemorySanitizer/experimental-reduce.ll
2

Should we rename this file to reduce.ll?

spatel accepted this revision.Wed, Oct 7, 10:00 AM

LGTM

llvm/docs/LangRef.rst
15595

It's redundant/unnecessary to mention 'fast' here (and in the fmul overview) because 'fast' is just an alias for all FMF including 'reassoc'.

This revision is now accepted and ready to land.Wed, Oct 7, 10:00 AM

I'll make those last changes in the commit.

Thanks to everyone who's pushed this forward over the last few years with fixes and reviews!

This revision was landed with ongoing or failed builds.Wed, Oct 7, 10:37 AM
This revision was automatically updated to reflect the committed changes.

I think there may be a small remaining issue on the llvmir-intrinsics.mlir test on Windows, as seen here: http://lab.llvm.org:8011/#/builders/82/builds/40/steps/7/logs/stdio

$ ":" "RUN: at line 1"
$ "e:\build_slave\mlir-x64-windows-ninja\build\bin\mlir-translate.exe" "-mlir-to-llvmir" "E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\test\Target\llvmir-intrinsics.mlir"
# command stderr:
Expected an argument of Vector Type
UNREACHABLE executed at E:\build_slave\mlir-x64-windows-ninja\llvm-project\llvm\lib\IR\Function.cpp:1167!

I think there may be a small remaining issue on the llvmir-intrinsics.mlir test on Windows, as seen here: http://lab.llvm.org:8011/#/builders/82/builds/40/steps/7/logs/stdio

$ ":" "RUN: at line 1"
$ "e:\build_slave\mlir-x64-windows-ninja\build\bin\mlir-translate.exe" "-mlir-to-llvmir" "E:\build_slave\mlir-x64-windows-ninja\llvm-project\mlir\test\Target\llvmir-intrinsics.mlir"
# command stderr:
Expected an argument of Vector Type
UNREACHABLE executed at E:\build_slave\mlir-x64-windows-ninja\llvm-project\llvm\lib\IR\Function.cpp:1167!

Yep, I've just pushed c1247f0e74bff00ab9a896a8132318916f3e84a7 to hopefully fix that.