Page MenuHomePhabricator

[RISCV] Support STEP_VECTOR with a step greater than one
ClosedPublic

Authored by frasercrmck on Tue, Apr 20, 7:41 AM.

Details

Summary

DAGCombiner was recently taught how to combine STEP_VECTOR nodes,
meaning the step value is no longer guaranteed to be one by the time it
reaches the backend for lowering.

This patch supports such cases on RISC-V by lowering to other step
values to a multiply following the vid.v instruction. It includes a
small optimization for common cases where the multiply can be expressed
as a shift left.

Diff Detail

Event Timeline

frasercrmck created this revision.Tue, Apr 20, 7:41 AM
frasercrmck requested review of this revision.Tue, Apr 20, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 20, 7:41 AM

D100812 has checked in. FYI

I hit this one recently, too!

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

I used DAG.getSplatVector here (I guess makes little difference) but I didn't consider the power-of-two case.

I'm curious that we cannot combine a case like this

  t13: nxv1i64 = RISCVISD::VID_VL t12, Register:i64 $x0
  t14: nxv1i64 = splat_vector Constant:i64<2>
t15: nxv1i64 = mul t13, t14

into

  t13: nxv1i64 = RISCVISD::VID_VL t12, Register:i64 $x0
  t16: nxv1i64 = splat_vector Constant:i64<1>
t15: nxv1i64 = shl t13, t16
frasercrmck added inline comments.Thu, Apr 29, 1:30 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3812

I don't think we can use getSplatVector because at this stage for i64 vectors on RV32, we can't introduce an illegal i64 type and so it would be nxvXi64 = splat_vector i32 which is ill-formed.

Regarding the second part, I think with splat_vector that would be possible at this stage -- it relies on getNode(ISD::MUL) folding constant arithmetic as we're post-combine -- but as I mentioned we can't use that node.

Perhaps if we conditionally used splat_vector and splat_vector_parts it would work, but splat_vector_parts isn't yet hooked up to the optimization machinery. I think it's better to do this awkward MUL/SHL lowering ourselves so that rv32 and rv64 behave the same way, but I can appreciate the DRY side of the argument.

rogfer01 accepted this revision.Thu, Apr 29, 11:02 PM

LGTM. Thanks @frasercrmck !

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

RIght, thanks for the explanation I had forgotten about rv32.

This revision is now accepted and ready to land.Thu, Apr 29, 11:02 PM