Page MenuHomePhabricator

Add rsqrt op to Standard dialect and lower it to LLVM dialect.
ClosedPublic

Authored by akuegel on Feb 28 2020, 7:01 AM.

Details

Reviewers
ftynse
herhut
Summary

This adds an rsqrt op to the standard dialect, and lowers
it as 1 / sqrt to the LLVM dialect.

Diff Detail

Event Timeline

akuegel created this revision.Feb 28 2020, 7:01 AM
akuegel updated this revision to Diff 247259.Feb 28 2020, 7:09 AM

Fix ClangTidy warnings.

ftynse requested changes to this revision.Feb 28 2020, 7:09 AM

Mostly good, let's make sure it works for other types than f32.

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
1688

This may not necessarily work if the operand type is not float. Maybe you could dispatch to different attributes based on type?

mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir
23

Could we also have a test for f64/double?

This revision now requires changes to proceed.Feb 28 2020, 7:09 AM
akuegel updated this revision to Diff 247600.Mar 2 2020, 4:25 AM
akuegel marked an inline comment as done.

Don't hard-code constant to have type F32.

akuegel marked 2 inline comments as done.Mar 2 2020, 4:27 AM
akuegel added inline comments.
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
1688

I tried to fix this by using getFloatAttr(operandType, 1.0) instead. Not sure whether operandType is the actual type it expects here though. Now I get llvm.float / llvm.double as types for the constants, before it was f32.

akuegel marked an inline comment as done.Mar 2 2020, 4:35 AM
akuegel added inline comments.
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
1688

Ah, and if I run the test without -c opt, I also get an assert failure. So as I feared, this is not the right fix yet. Will try to figure out how to make it use the F32 / F64 type as required.

akuegel updated this revision to Diff 247611.Mar 2 2020, 5:20 AM

Fix type issues.

akuegel marked an inline comment as done.Mar 2 2020, 5:24 AM

PTAL. The tests seem to work now.

mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir
48

Does this look ok? I am not sure whether the operand type of constant is allowed to be different from the result type. But seems there is no assert that is triggered, so maybe it supports broadcasts?

ftynse added inline comments.Mar 2 2020, 5:54 AM
mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir
48

I think you need something like llvm.mlir.constant(dense<1.0> : vector<4xf32>) : !llvm<"<4 x float>">. We have SplatElementsAttr to broadcast constants in a vector, which is printed like that.

akuegel updated this revision to Diff 248142.Mar 4 2020, 3:52 AM

Handle vector types.

akuegel marked an inline comment as done.Mar 4 2020, 3:55 AM

Sorry for not following up sooner, I was busy with other stuff yesterday. Today, with some help by Stephan I was able to make this work. I copied the handling with unrolling multi-dimensional vectors from the generic elementwise op handling. This could potentially be refactored, especially I wonder whether Tanh should also use this.

ftynse accepted this revision.Mar 4 2020, 4:06 AM

I copied the handling with unrolling multi-dimensional vectors from the generic elementwise op handling. This could potentially be refactored, especially I wonder whether Tanh should also use this.

I think I've seen a commit that removed the default tanh lowering recently. Anyway, a refactoring is welcome if you think it makes sense, in a separate commit.

This revision is now accepted and ready to land.Mar 4 2020, 4:06 AM
akuegel closed this revision.Mar 5 2020, 6:32 AM

Landed as revision 91acb5b3e1c3
Unforunately I forgot to reset the link to this review, after I had accidentally created https://reviews.llvm.org/D75353

rriddle added inline comments.Mar 5 2020, 10:49 AM
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
1676

Don't use dyn_cast_or_null here, the result of getType is never null. Use dyn_cast if you know the input is non-null.

akuegel marked an inline comment as done.Mar 6 2020, 4:11 AM
akuegel added inline comments.
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
1676

Thanks for the suggestion :)
I made that part of a refactoring CL that I was planning to do, anyway: https://reviews.llvm.org/D75733