This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add scalable vector icmp ISel patterns
ClosedPublic

Authored by frasercrmck on Jan 6 2021, 4:56 AM.

Details

Summary

Original patch by @rogfer01.

The RVV integer comparison instructions are defined in such a way that
many LLVM operations are defined by using the "opposite" comparison
instruction and swapping the operands. This is done in this patch in
most cases, except for the mappings where the immediate range must be
adjusted to accomodate:

  • va < i --> vmsle{u}.vi vd, va, i-1, vm
  • va >= i --> vmsgt{u}.vi vd, va, i-1, vm

That is left for future optimization; this patch supports all operations
but in the case of the missing mappings the immediate will be moved to
a scalar register first.

Since there are so many condition codes and operand cases to check, it
was decided to reduce the test burden by only testing the "vscale x 8"
vector types.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Fraser Cormack <fraser@codeplay.com>

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 6 2021, 4:56 AM
frasercrmck requested review of this revision.Jan 6 2021, 4:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2021, 4:56 AM

I'm perhaps testing too many immediates here and should reduce the testing of edge-cases to those instructions that will actually make use of them.

  • clarify comment a little

fix an operand class and fix a comment

craig.topper added inline comments.Jan 6 2021, 10:05 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
232 ↗(On Diff #314915)

What is the splat for VX/VI is on the LHS?

frasercrmck added inline comments.Jan 7 2021, 7:53 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
232 ↗(On Diff #314915)

Ah, good spot. I had assumed constants were canonicalized to the RHS like they are with scalar/fixed-length vectors.

I've now updated llvm::isConstOrConstSplat to handle SPLAT_VECTOR and it "just works". I also see further optimization such as for uge v, 0:

-; CHECK-NEXT:    vmv.v.i v25, 0
-; CHECK-NEXT:    vmsleu.vv v0, v25, v16
+; CHECK-NEXT:    vmset.m v0

That doesn't solve the problem for VX though. I'd probably suggest a custom combine for that, to avoid too much pattern complication. Any thoughts about that?

  • Support SPLAT_VECTOR in llvm::isConstOrConstSplat
  • Adjust RISCV and AArch64 tests accordingly
frasercrmck added a subscriber: c-rhodes.

@c-rhodes, could you possibly check that the changes to the AArch64 tests are okay? The whilelo one is a bit confusing.

  • improve test coverage for setcc with VX/VI on LHS
  • cut down on uninteresting VI test cases to compensate

@c-rhodes, could you possibly check that the changes to the AArch64 tests are okay? The whilelo one is a bit confusing.

LGTM! The whilelo is an all false predicate

craig.topper added inline comments.Jan 7 2021, 2:48 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9139

Any special reason this bitsEq but the build_vector code is operator==?

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
232 ↗(On Diff #314915)

There's a small possibility of an infinite loop if the generic combines don't know the rule being used. They might end up converting it back and forth. For example, there's a combine that tries to canonicalize setcc with non-constant operands to have the same operand order as an existing isd::sub. Which probably only make sense for scalars where sub produces flags on some targets.

Can you add a FIXME for VX LHS? I think this patch can go in and we can address it as follow up.

frasercrmck added inline comments.Jan 8 2021, 1:37 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9139

Err no, I think that's a remnant of me trying something else out. I'll fix that up.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
232 ↗(On Diff #314915)

Yeah good point. I had woken up today hoping we could just do it in the same place as the constant canonicalization with a target hook (prefersSplatOnRHS(Opcode, Operand) or something) if AArch64 would also benefit from it. It appears they're doing it manually in patterns. It might still reduce their pattern load, but it has taken the impetus out of that idea somewhat.

But for now, I'll enhance the tests and add a FIXME to them like I did with the fp tests.

  • update type check
  • add scalar/vector fixme to tests
  • fix an infinite loop with setcc + two constant splats
  • add a regression test for it

update comment a little

craig.topper accepted this revision.Jan 8 2021, 10:03 AM

LGTM with that one auto changed.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9135

Use EVT here instead of auto.

This revision is now accepted and ready to land.Jan 8 2021, 10:03 AM
This revision was landed with ongoing or failed builds.Jan 9 2021, 1:02 PM
This revision was automatically updated to reflect the committed changes.