Page MenuHomePhabricator

[RISCV] Add DAG combines to transform ADD_VL/SUB_VL into widening add/sub.
ClosedPublic

Authored by craig.topper on Jan 22 2022, 9:47 AM.

Details

Summary

This adds or reuses ISD opcodes for vadd.wv, vaddu.wv, vadd.vv, vaddu.vv
and a similar set for sub.

I've included support for narrowing scalar splats that have known
sign/zero bits similar to what was done for MUL_VL.

The conversion to vwadd.vv proceeds in two phases. First we'll form
a vwadd.wv by narrowing one of the operands. Then we'll visit the
vwadd.wv to try to narrow the other operand. This turned out to be
simpler than catching all the cases in one step. The forming of of
vwadd.wv can happen for either operand for add, but only the right
hand side for sub since sub isn't commutable.

An interesting quirk is that ADD_VL and VZEXT_VL/VSEXT_VL are formed
during vector op legalization, but VMV_V_X_VL isn't usually formed
until op legalization when BUILD_VECTORS are handled. This leads to
VWADD_W_VL forming in one DAG combine round, and then a later DAG combine
round sees the VMV_V_X_VL and needs to commute the operands to get the
splat in position. This alone necessitated a VWADD_W_VL combine function
which made forming vwadd.vv in two stages an easy choice.

I've left out trying hard to form vwadd.wx instructions for now. It would
only save an extend in the scalar domain which isn't as interesting.

Might need to review the test coverage a bit. Most of the vwadd.wv
instructions are coming from vXi64 tests on rv64. The tests were
copy pasted from the existing multiply tests.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 22 2022, 9:47 AM
craig.topper requested review of this revision.Jan 22 2022, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2022, 9:47 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
eopXD added inline comments.Jan 22 2022, 10:06 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7429

Why not just directly fetch with getOpCode and have assertions in the beginning of the function just like the previous one?

craig.topper added inline comments.Jan 22 2022, 10:34 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7429

I don't think I understand the question. This switch mainly exists to convert from one opcode to another.

eopXD added inline comments.Jan 22 2022, 10:35 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7429

I misread the code. I thought it was just direct copying. My bad.

Just to confirm, this is not fixed vector specific, is it?

If this is the case could you add some tests for those?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7381

I'm confused by this reference to "widening multiply". I fail to see anything about widening multiplies in this patch.

7465

Ditto.

craig.topper added inline comments.Jan 27 2022, 12:48 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7381

Copy paste from the widening mul function.

Just to confirm, this is not fixed vector specific, is it?

If this is the case could you add some tests for those?

I think we only use ADD_VL and SUB_VL for fixed vectors, and VP intrinsics today. And we use VZEXT_VL and VSEXT_VL for fixed and scalable vectors, but we don't have VP intrinsics for them yet.

So I think this code is fixed vector restricted right now because only fixed vectors use both ADD_VL/SUB_VL and VZEXT_VL/VSEXT_VL. Does that sound right, @frasercrmck ?

Fix copy/paste comment mistake

Just to confirm, this is not fixed vector specific, is it?

If this is the case could you add some tests for those?

I think we only use ADD_VL and SUB_VL for fixed vectors, and VP intrinsics today. And we use VZEXT_VL and VSEXT_VL for fixed and scalable vectors, but we don't have VP intrinsics for them yet.

So I think this code is fixed vector restricted right now because only fixed vectors use both ADD_VL/SUB_VL and VZEXT_VL/VSEXT_VL. Does that sound right, @frasercrmck ?

Yeah that sounds about right. I've been wondering if we should try and unify it all but I fear custom lowering add and co. would prohibit too many optimizations.

rogfer01 accepted this revision.Feb 2 2022, 2:35 AM

Ok thanks for the clarifications!

LGTM then.

This revision is now accepted and ready to land.Feb 2 2022, 2:35 AM
This revision was landed with ongoing or failed builds.Feb 2 2022, 10:15 AM
This revision was automatically updated to reflect the committed changes.