This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Scalarize extend/truncate for splat vector.
ClosedPublic

Authored by jacquesguan on Apr 1 2022, 12:10 AM.

Details

Summary

This revision scalarizes extend/truncate for splat vector.

Diff Detail

Event Timeline

jacquesguan created this revision.Apr 1 2022, 12:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 12:10 AM
jacquesguan requested review of this revision.Apr 1 2022, 12:10 AM
RKSimon added inline comments.Apr 6 2022, 4:30 AM
llvm/test/CodeGen/RISCV/rvv/vfwmacc-sdnode.ll
28 ↗(On Diff #419646)

Any luck with these regressions?

llvm/test/CodeGen/RISCV/rvv/vwsub-sdnode.ll
2–3

You need to re-add triple specific checks now that some of these test results diverge

; RUN: llc -mtriple=riscv32 -mattr=+v -verify-machineinstrs < %s | FileCheck %s --check-prefixes=CHECK,CHECK32
; RUN: llc -mtriple=riscv64 -mattr=+v -verify-machineinstrs < %s | FileCheck %s --check-prefixes=CHECK,CHECK64
craig.topper added inline comments.Apr 6 2022, 8:49 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
437

I don't think UnOp is the right term here. UnaryOperator in IR is the base clang used by fneg. CastOp would probably be better.

Address comment.

jacquesguan added inline comments.Apr 6 2022, 11:40 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
437

Done, thanks.

llvm/test/CodeGen/RISCV/rvv/vfwmacc-sdnode.ll
28 ↗(On Diff #419646)

Done, thanks.

llvm/test/CodeGen/RISCV/rvv/vwsub-sdnode.ll
2–3

Done, thanks.

RKSimon added inline comments.Apr 8 2022, 10:48 AM
llvm/test/CodeGen/RISCV/rvv/vwsub-sdnode.ll
548

regressions?

craig.topper added inline comments.Apr 8 2022, 11:05 AM
llvm/test/CodeGen/RISCV/rvv/vwsub-sdnode.ll
548

RISC-V is weak on scalar zero extend in the base ISA. We get a zext.w operation with the Zba extension. On the vector side a widening vwsub.wx is probably more expensive than a non-widening vsub.vx

RKSimon added inline comments.Apr 8 2022, 11:12 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
23091

Should we be extracting from a SPLAT_VECTOR node like this or should we be accessing (and maybe truncating?) the N0.getOperand(0) directly?

Address comment.

jacquesguan added inline comments.Apr 10 2022, 8:36 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
23091

Thanks, I changed it to the latter one.

Sorry, I'm a bit rusty. What happens with the equivalent fixed-length vector splats (BUILD_VECTOR)? If they're scalarized can we do the same for SPLAT_VECTOR in the same place? If they're not scalarized, what's the logic for only doing this transform for SPLAT_VECTOR?

rebase main and address comment.

Sorry, I'm a bit rusty. What happens with the equivalent fixed-length vector splats (BUILD_VECTOR)? If they're scalarized can we do the same for SPLAT_VECTOR in the same place? If they're not scalarized, what's the logic for only doing this transform for SPLAT_VECTOR?

I changed some code to support other kinds splat.

RKSimon added inline comments.Aug 17 2022, 3:26 AM
llvm/test/CodeGen/RISCV/rvv/vnsra-sdnode.ll
33

Still a regression - can RISCV64 really not be coaxed into generating something better?

Disable salarize sign_ext and zero_ext for RISCV target.

jacquesguan added inline comments.Aug 18 2022, 1:45 AM
llvm/test/CodeGen/RISCV/rvv/vnsra-sdnode.ll
33

In order to generate widening instruction, I disable scalarizing of sign_ext and zero_ext, because the scalar type integer extention is easy to combine with other node, make it won't match the pattern.

cheers - its up to the RISCV guys as to whether this is acceptable (and whether it should be pulled out as a parent patch?)

cheers - its up to the RISCV guys as to whether this is acceptable (and whether it should be pulled out as a parent patch?)

@craig.topper @frasercrmck What do you about this way, or any advice?

frasercrmck added inline comments.Sep 1 2022, 7:41 AM
llvm/test/CodeGen/RISCV/rvv/vnsra-sdnode.ll
33

Do you think there's any way to fix it?

jacquesguan added inline comments.Sep 1 2022, 11:53 PM
llvm/test/CodeGen/RISCV/rvv/vnsra-sdnode.ll
33

I think if we change the widen pattern to DAG combiner, maybe could generte some, but common DAG folder may fold extend to for others example: ext_inreg firstly. So more condition we need to consider

RKSimon accepted this revision.Dec 8 2022, 10:03 AM

LGTM - cheers

This revision is now accepted and ready to land.Dec 8 2022, 10:03 AM

@jacquesguan Please can you take a look at https://github.com/llvm/llvm-project/issues/62234 thats related to this patch?

@jacquesguan Please can you take a look at https://github.com/llvm/llvm-project/issues/62234 thats related to this patch?

OK.