This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Enable cross basic block aware vsetvli insertion
ClosedPublic

Authored by craig.topper on May 18 2021, 5:02 PM.

Details

Summary

This patch extends D102737 to allow VL/VTYPE changes to be taken
into account before adding an explicit vsetvli.

We do this by using a data flow analysis to propagate VL/VTYPE
information from predecessors until we've determined a value for
every value in the function.

We use this information to determine if a vsetvli needs to be
inserted before the first vector instruction the block.

Diff Detail

Event Timeline

craig.topper created this revision.May 18 2021, 5:02 PM
craig.topper requested review of this revision.May 18 2021, 5:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 5:02 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

Add phase 1 and 2 on top of phase 3 with all insertion done in phase 3.

This makes phase 1 and phase 3 look very similar since we still need to
determine the basic block's effect even if we don't insert anything.

Rebase and move some common logic into a helper function.

craig.topper edited the summary of this revision. (Show Details)May 24 2021, 1:13 PM
craig.topper edited the summary of this revision. (Show Details)

Don't update CurInfo from predecessor in phase 3 unless we inserted a vsetvli.

Mostly just stylistic things from me at this point. I've written a similar pass before so this is surprisingly familiar.

I do also think some new MIR tests to cover this functionality would be good though.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
13

Wee typo on informatiom

161

Super nit but a blank line between these functions is probably recommended.

179

Is there a reason not to make the constructor call setUnknown?

I was wondering if a static VSETVLIInfo::getUnknown-style function might improve some use cases. Here would just be return VSETVLIInfo::getUnknown(); for instance.

479

For some reason the lack of a blank line above this stands out to me.

craig.topper updated this revision to Diff 347747.EditedMay 25 2021, 11:48 AM

Address spelling and style comments.
Use a reference to BlockInfo[ThisBlock] to shorten code.
Add getUnkown

I'll work on MIR tests.

Add MIR tests and some more interesting ll tests.

frasercrmck accepted this revision.May 26 2021, 6:44 AM

LGTM.

llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
86 ↗(On Diff #347850)

Interesting. I was kinda hoping the middle-end would hoist it, but I guess that's not possible because the intrinsic is marked as having side effects. I looked at that a while ago when a vsetvli intrinsic was preventing two identical loads on either side being merged, and was wondering if we might something less nuclear than IntrHasSideEffects, because that seems to negate IntrNoMem in useful optimizations.

This revision is now accepted and ready to land.May 26 2021, 6:44 AM