This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use the first element of source as the start value of reduction.
ClosedPublic

Authored by fakepaper56 on Jul 21 2023, 2:34 AM.

Details

Summary

Previously when llvm.reduce.* lowered, riscv backend created scalar vector with
netural element as start value. For llvm.reduce.and/or/min/max/fmax/fmin, we
could use the first element of source as the start value. It's benefit for RVV
since we could just use source vector as start vector.

Diff Detail

Event Timeline

fakepaper56 created this revision.Jul 21 2023, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2023, 2:34 AM
fakepaper56 requested review of this revision.Jul 21 2023, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2023, 2:34 AM
luke added a comment.Jul 21 2023, 3:02 AM

I can't speak for the fmin/fmax parts, but for the integer ops this makes sense to me

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7661–7662

What happens it it's not converted to a scalable vector?

fakepaper56 added inline comments.Jul 21 2023, 8:38 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3467

What happens it it's not converted to a scalable vector?

It just that fixed vectors are hard to compares with scalable vector type here.

7661–7662

Thank you for the feedback.

Address Luke's comment and remove unused variable.

fakepaper56 planned changes to this revision.Jul 22 2023, 7:59 PM

The patch will encounter ICE when using extractelement for the start of vp.reduce.*.

fakepaper56 updated this revision to Diff 543245.EditedJul 22 2023, 9:37 PM

This update does 3 things:

  1. Since (extractelement fixed_vec, 0) may be a start of vp.reduce, we need to handle this for lowerScalarInsert.
  2. Remove function getFirstElement().
  3. Add test CodeGen/RISCV/rvv/combine-extractelement-vp-reduce.ll for 1.
craig.topper added inline comments.Jul 24 2023, 1:19 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3475

Is VT here always scalable?

fakepaper56 added inline comments.Jul 24 2023, 6:03 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3475

I think all lowerScalarInsert calls use scalable vector VT and it looks like lowerScalarInsert can't have fixed vector VT.

craig.topper added inline comments.Jul 24 2023, 6:28 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3475

can you add an assertion?

Add assertion to check whether VT is scalable vector type.

fakepaper56 marked an inline comment as done.Jul 24 2023, 7:54 PM
This revision is now accepted and ready to land.Jul 31 2023, 1:30 PM
This revision was landed with ongoing or failed builds.Jul 31 2023, 10:15 PM
This revision was automatically updated to reflect the committed changes.