Page MenuHomePhabricator

[mlir][Linalg] Add support for min/max reduction vectorization in linalg.generic
ClosedPublic

Authored by dcaballe on Sep 30 2021, 10:08 AM.

Details

Summary

This patch extends Linalg core vectorization with support for min/max reductions
in linalg.generic ops. It enables the reduction detection for min/max combiner ops.
It also renames MIN/MAX combining kinds to MINS/MAXS to make the sign explicit for
floating point and signed integer types. MINU/MAXU should be introduce din the future
for unsigned integer types.

Diff Detail

Event Timeline

dcaballe created this revision.Sep 30 2021, 10:08 AM
dcaballe requested review of this revision.Sep 30 2021, 10:08 AM
ThomasRaoux added inline comments.Sep 30 2021, 10:18 AM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

nit: The naming seems a bit confusing. Is there any reason why we don't want to add spearate minf/maxf? It doesn't look like it would add code/complexity?

mlir/lib/Dialect/Vector/VectorTransforms.cpp
871–873

This lowering doesn't propagate Nan while the new minf op is supposed to propagate Nan. I assume we want to match the semantic of minf. (same for maxf of course)

ThomasRaoux added inline comments.Sep 30 2021, 10:24 AM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
871–873

The most natural would be to lower to minf/maxf ops in my opinion.

pifon2a added inline comments.Sep 30 2021, 1:14 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

yes, i think with the current maxf, maxsi, maxui ops added we can actually have 1 to 1 map.

mlir/lib/Dialect/Vector/VectorTransforms.cpp
823–825

create<MinSIOp>?

826–827

create<MaxSIOp>?

871–873

+1 @ThomasRaoux , create<MinFOp>?

dcaballe added inline comments.Sep 30 2021, 3:16 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

What I inferred from the current implementation, and maybe I'm totally wrong, is that the actual type is taken from the reduction/contraction op itself. I understand that's why we only have ADD, MUL, etc., and not ADDI/ADDF, MULI/MULF etc. The only thing that is missing is the signedness for those operations where it makes a difference (min/max, in this case). I though adding MINS/MAXS for signed integer and floating point (and eventually MINU/MAXU for unsigned integers) would avoid duplicating all the I/SI/UI/F variants.

If we wanted to go with all the I/SI/UI/F variants, shouldn't we do it for all the ops to be consistent? We would also need verification rules to make sure the type of the combiner matches the type of the op. Again, this sounds a bit redundant to me since the type itself is in the operation. We just need the sign information.

WDYT?

mlir/lib/Dialect/Vector/VectorTransforms.cpp
871–873

Yes, that makes sense, thanks! I thought @pifon2a's patch was taking care of that already. I see now that it's only expanding the min/max ops to the compare/select implementation.

ThomasRaoux added inline comments.Sep 30 2021, 4:16 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

I agree, that's how it is right now, however having MAXS maps to float max seems confusing to me. Overall even for Add/Mul the float and integer version are separate opcodes everywhere so we don't really save a lot having a single enum.
It doesn't make sense to have ADD and MUL for I/SI/UI as they are equivalent so I don't think it is makes sense to have different version for those (and we don't have different opcodes for them).

Updating verifier is just a matter of updating is SupportedCombiningKind so shouldn't be to bad. What do you think?

dcaballe added inline comments.Sep 30 2021, 6:05 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

It doesn't make sense to have ADD and MUL for I/SI/UI as they are equivalent so I don't think it is makes sense to have different version for those (and we don't have different opcodes for them).

Sorry, I wasn't clear here. Not all the variants make sense for all the ops, obviously. I and SI/UI are mutually exclusive.

No strong opinion, though. I can start addressing the min/max ones.

ThomasRaoux added inline comments.Sep 30 2021, 9:02 PM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

I don't have a strong opinion but my preference is to have mins/maxs/minf/maxf in this case

General comment (not for this CL), I find mins maxs etc quite worse than smin, smax etc.
Any reason we want to deviate from the LLVM names (e.g. https://llvm.org/docs/LangRef.html#llvm-smin-intrinsic) ?

I see that the attr names are consistent with the MLIR names but I don't know why we deviated in the first place.

mlir/include/mlir/Dialect/Vector/VectorOps.td
41

The LLVM Langref has smin/smax/umin/umax/fmin/fmax; any reason we want to deviate from that ?
@pifon2a I realized I hadn't checked that in your original PR, but would be nice to be consistent with LLVM unless there is a strong reason not to.

I'd agree that using smin/smax for float would be a further step away form LLVM so I'd prefer we'd avoid that.

pifon2a added inline comments.Oct 1 2021, 12:17 AM
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

Because in MLIR we already have "postfix" style. We have addf and not fadd. Also the PR that adds Arith dialect (https://reviews.llvm.org/D110200) has DivSIOp and so on. I wanted to be consistent with the style.

dcaballe planned changes to this revision.Oct 1 2021, 10:09 AM
dcaballe added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
41

Yes, I think it's important to keep things consistent within MLIR. If we want to align with LLVM, it's probably better to address it separately, at a broader level. I'll go with the S/U/F approach for min/max, then.

41

> The LLVM Langref has smin/smax/umin/umax/fmin/fmax; any reason we want to deviate from that ?

Note that we have more inconsistencies than just the prefix/postfix name. LLVM has the following intrinsics for fp min/max:

Scalar/vector max/min intrinsics:

Horizontal vector reductions:

So, to be aligned with LLVM, our minf/maxf ops should be renamed to minimum/maximum. Note as well that, AFAICT, there are no horizontal reductions with minimum/maximum semantics in LLVM so we can't use those!

If we want to be aligned with LLVM, I think we should consider renaming minf/maxf -> minimum/maximum.

dcaballe updated this revision to Diff 377075.Oct 4 2021, 8:09 PM

Addressed feedback.
Sorry for the delay but I went into a rabbit hole. For some reason,
the existing implementation lowered min/max ops on signless integers
assuming signed integers. It took me a while to realize what was going
on...

dcaballe added inline comments.Oct 4 2021, 8:14 PM
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
494

This is exactly the problem that I described above. This lowering is incorrect and it won't produce the right output if the input has NaNs. We need to add a new 'vector_reduce' intrinsic to LLVM or implement our own lowering to more basic vector instructions in MLIR. I'll follow up on this when we hit this limitation.

ThomasRaoux added inline comments.Oct 4 2021, 8:16 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
99

should MAXF case only return true if the elementType is float?

308–309

can we call isSupportedCombiningKind here?

dcaballe updated this revision to Diff 377093.Oct 4 2021, 11:07 PM
dcaballe marked 2 inline comments as done.

Addressed feedback.
Thanks!

mlir/lib/Dialect/Vector/VectorOps.cpp
99

Good catch, thanks!

308–309

much better, thanks!

pifon2a accepted this revision.Oct 5 2021, 4:29 AM

Great, thank you, Diego!

This revision is now accepted and ready to land.Oct 5 2021, 4:29 AM
ThomasRaoux accepted this revision.Oct 5 2021, 9:05 AM
ThomasRaoux added inline comments.
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
494

As discuss it is a bit scary to have a potential silent miscompile. Can we fail this pattern here?