Page MenuHomePhabricator

[RISCV] A vector instruction without a tail is always tail agnostic
Changes PlannedPublic

Authored by reames on Jun 8 2022, 11:48 AM.

Details

Summary

This change implements a ISEL peephole to replace the tail policy of a vector operation with no tail elements (because AVL is VLMAX) with tail agnostic. I believe this to be profitable (or at least neutral) on all reasonable hardware.

This could be reasonable done in a couple of different places. I considered doing this inside RISCVInsertVSETVLI.cpp since the true motivation is - once combined with another change - to eventually get rid of some vsetvli transitions, but this felt more like an instruction selection change.

A couple things for reviewers:

  • This is deliberately extremely generic. The offset code is modeled off what we do in RISCVInsertVSETVLI.cpp, but if there are special cases in SelectionDAG I don't know about, there might be a bug here.
  • I'm planning to extend this with a couple other transforms in the nearish future. The first is going to use VLMAX for AVL immediates we can prove to exceed VLMAX. The second is going to apply the same policy tweak for unmasked and mask ignoring instructions.

Diff Detail

Unit TestsFailed

TimeTest
730 msx64 debian > SanitizerCommon-tsan-x86_64-Linux.Linux::decorate_proc_maps.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -funwind-tables -ldl -g /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/decorate_proc_maps.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/decorate_proc_maps.cpp.tmp

Event Timeline

reames created this revision.Jun 8 2022, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 11:48 AM
reames requested review of this revision.Jun 8 2022, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 11:48 AM
craig.topper added inline comments.Jun 8 2022, 6:56 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
192

TSFlags would be unused on a release build suggest

assert(hasVecPolicyOp(Desc.TSFlags));
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2489

Assuming Subtarget is RISCVSubtarget*, doesn't RISCVSubtarget::getInstrInfo() return RISCVInstrInfo * without a cast?

2518

Just to confirm.

vcompress doesn't have a policy operand so this wouldn't apply? I ask because vcompress writes less than VL elements and the tail policy applies to the other elements.

reductions and vmv.s.x also don't have policy operand.

2532

Why not copy all values using the SmallVector range constructor and overwrite Ops[VecPolicyOpNum] afterwards.

reames added inline comments.Jun 9 2022, 5:04 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2489

I'd copied this from somewhere else, and only partially fixed it up. The source of my copy was fixed in 28be4b745.

2532

Good idea, will do.

reames updated this revision to Diff 435744.Jun 9 2022, 5:13 PM

Address most review comments - one pending, will reply to it shortly.

reames planned changes to this revision.Jun 9 2022, 5:14 PM
reames added inline comments.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2518

Just noting this question is still open. I'm going to get dig into each of these cases, but I have not done it yet.

reames added inline comments.Jun 13 2022, 8:34 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2518

Looking at vcompress, it looks like it does not currently have a policy operand, and thus the patch as written is correct on that case. However, thinking about it, I suspect this *should* have a policy operand. Right now, we're taking the tail policy from the tied register logic in computeInfoForInstr, but we can a) frankly do better, and b) that code should probably live in ISEL. Either way, I think we need to future proof a bit here.

Any suggestions for a reasonable name for "can write less than VL elements" or do we already have that somewhere? I'll add a helper function with some asserts for now.

(I did not look at reduction or vmv.s.x in detail, but given the findings on vcompress, guarding one should guard all.)