Page MenuHomePhabricator

Add loop unrolling and peeling preferences for RISCV
AcceptedPublic

Authored by mcberg2021 on Fri, Nov 12, 12:46 PM.

Details

Summary

Both these preference helper functions have initial support with this change. The loop unrolling preferences are set with initial settings to control thresholds, size and attributes of loops to unroll with some tuning done. The peeling preferences may need some tuning as well as the initial support looks much like what other architectures utilize. An unrolling test is added for RISCV as well to track how preferences modify/control loop unrolling.

Diff Detail

Event Timeline

mcberg2021 created this revision.Fri, Nov 12, 12:46 PM
mcberg2021 requested review of this revision.Fri, Nov 12, 12:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 12, 12:46 PM
craig.topper added inline comments.Fri, Nov 12, 1:05 PM
llvm/test/CodeGen/RISCV/unroll.ll
9 ↗(On Diff #386925)

Drop the FunctionAttrs comment and the dso_local and local_unnamed_addr

169 ↗(On Diff #386925)

I doubt we need all these attributes and metadata

craig.topper added inline comments.Fri, Nov 12, 1:06 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
204

The MVE in this comment appears to have been copied from ARM.

jrtc27 added a comment.EditedFri, Nov 12, 1:06 PM

This test screams of "I took C and shoved it into a test case" rather than actually taking the time to distill it down to a minimal example of IR free from irrelevant clutter

llvm/test/CodeGen/RISCV/unroll.ll
2 ↗(On Diff #386925)

opt test in CodeGen?

jrtc27 added inline comments.Fri, Nov 12, 1:08 PM
llvm/test/CodeGen/RISCV/unroll.ll
2 ↗(On Diff #386925)

(this belong in llvm/test/Transforms/LoopUnroll/RISCV)

Updated as per feedback.

mcberg2021 marked an inline comment as done.Fri, Nov 12, 1:45 PM
mcberg2021 marked 4 inline comments as done.
mcberg2021 added inline comments.
llvm/test/CodeGen/RISCV/unroll.ll
2 ↗(On Diff #386925)

Test moved to LoopUnroll/RISCV

mcberg2021 marked an inline comment as done.

Missed an update...

jrtc27 added inline comments.Sat, Nov 13, 12:18 PM
llvm/test/Transforms/LoopUnroll/RISCV/unroll.ll
3

Use -mtriple=riscv64 unless it's genuinely OS-dependent (which this is not)

5

I doubt you need all these pointer attributes

147

These comments do nothing

147

This would also be more natural as the final block in the function; presumably the current block schedule is an artefact of the order in which various optimisation happened on the original C and IR

164

Is the TBAA actually needed?

Updated with simplifications and formatting.

mcberg2021 marked 5 inline comments as done.Sun, Nov 14, 12:19 PM

Marked tasks as done.

jrtc27 added inline comments.Sun, Nov 14, 12:25 PM
llvm/test/Transforms/LoopUnroll/RISCV/unroll.ll
147

This comment is still there

164

!0 is only used self-referentially; only !1 is referenced from outside of the metadata itself. So I don't think this distinct does anything.

165

Is this needed (same for the !llvm.loop)? If yes, just inline it like !llvm.loop, if no delete them.

More cleanup

mcberg2021 marked 3 inline comments as done.Sun, Nov 14, 3:44 PM
frasercrmck added inline comments.Mon, Nov 15, 2:00 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
204

Is this truly checking "vectorized loops" or just loops containing vector instructions? We've already checked that the loop isn't vectorized according to metadata. What about code written using RVV intrinsics, or with OpenCL/SYCL/etc? We might want to unroll those loops, right?

217

Does the explicit size of 4 help much or should we just use SmallVector<const Value*>?

khchen added a subscriber: khchen.Mon, Nov 15, 4:34 AM
mcberg2021 added inline comments.Mon, Nov 15, 9:10 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
204

I think for now I am going to mark this with a TODO for more tuning, I updated the comment for vectorized instructions, it will be uploaded soon...

217

This setting mirrors SLP's generic vector operand setting, which we utilize, so it does seem appropriate.

craig.topper added inline comments.Mon, Nov 15, 9:11 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
204

This part of the patch came from a change I made internally. I believe this entire loop was just naively copied from the ARM target.

217

This was also copied from ARM.

Updated comments as needed.

mcberg2021 marked 4 inline comments as done.Mon, Nov 15, 9:20 AM
craig.topper added inline comments.Mon, Nov 15, 9:21 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
217

This shouldn't be mirroring anything. The ARM code I copied from pre-dates SmallVector's ability to automatically pick a value for the second template parameter. I think it will automatically pick a value more than 4. So I think the second parameter can be removed.

Removed size constraint on initialization of Operands to be consumed in getUserCost.

mcberg2021 marked an inline comment as done.Mon, Nov 15, 9:36 AM

Are there any further concerns? If not can we progress towards approval?

This revision is now accepted and ready to land.Fri, Nov 19, 10:00 AM

Perhaps we should run this across a set of benchmarks we're interested in?

Perhaps we should run this across a set of benchmarks we're interested in?

We've been using this internally on our SiFive 7 series and U8. Should we check the CPU?

Perhaps we should run this across a set of benchmarks we're interested in?

We've been using this internally on our SiFive 7 series and U8. Should we check the CPU?

I know that @asb often runs benchmarks over prospective patches, so I thought he might have some thoughts about we generally know this sort of thing is ready to go.