This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Enable reduction pattern SelectICmp and SelectFCmp.
ClosedPublic

Authored by Mel-Chen on Nov 14 2022, 5:56 AM.

Diff Detail

Event Timeline

Mel-Chen created this revision.Nov 14 2022, 5:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 5:56 AM
Mel-Chen requested review of this revision.Nov 14 2022, 5:56 AM
david-arm added inline comments.Nov 14 2022, 6:58 AM
llvm/test/Transforms/LoopVectorize/RISCV/select-cmp-reduction.ll
10

It might be worth making sure that some of the variants in Transforms/LoopVectorize/AArch64/sve-select-cmp.ll also work for RISCV? In particular, @pred_select_const_i32_from_icmp since that exposed a bug when I first landed support for this idiom.

Mel-Chen updated this revision to Diff 476448.Nov 18 2022, 6:01 AM

Add new test cases that are copied from Transforms/LoopVectorize/AArch64/sve-select-cmp.ll.

This revision is now accepted and ready to land.Nov 18 2022, 7:32 AM
reames added a subscriber: reames.Nov 18 2022, 7:34 AM

LGTM

Just to check, you've confirmed the actual codegen for this looks vaguely reasonable right? I don't see anything in the IR which worries me too much, just asking for the confirmation.

LGTM

Just to check, you've confirmed the actual codegen for this looks vaguely reasonable right? I don't see anything in the IR which worries me too much, just asking for the confirmation.

It does to me yeah - the output looks similar to llvm/test/Transforms/LoopVectorize/AArch64/sve-select-cmp.ll and we have the same pattern of compares/selects in the vector loop, as well as compares + or reduce in the middle block. None of the tests crash and the RISCV version bails out in the same way as AArch64 for @select_const_f32_from_icmp.

The tests in this patch don't test interleaving like the tests in llvm/test/Transforms/LoopVectorize/AArch64/sve-select-cmp.ll do, but I figured that they are going down the same code path by this point and wasn't sure if it would add much value being tested again here.

LGTM

Just to check, you've confirmed the actual codegen for this looks vaguely reasonable right? I don't see anything in the IR which worries me too much, just asking for the confirmation.

Codegen looks good.
Here is how I use to confirm codegen:

opt -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -scalable-vectorization=on -S Transforms/LoopVectorize/RISCV/select-cmp-reduction.ll > select-cmp-reduction.gen.ll
llc -mtriple=riscv64 -mattr=+m,+f,+v -verify-machineinstrs -target-abi=lp64d -riscv-v-vector-bits-min=-1 < select-cmp-reduction.gen.ll > select-cmp-reduction.gen.s