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.



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

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

TSFlags would be unused on a release build suggest


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


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.


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

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


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.

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

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.)