This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][LSR] Treat number of instructions as dominate factor in LSR cost decisions
ClosedPublic

Authored by reames on Jan 20 2023, 8:49 AM.

Details

Summary

This matches the behavior from a number of other targets, including e.g. X86. This does have the effect of increasing register pressure slightly, but we have a relative abundance of registers in the ISA compared to other targets which use the same heuristic.

The motivation here is that our current cost heuristic treats number of registers as the dominant cost. As a result, an extra use outside of a loop can radically change the LSR result. As an example consider test4 from the recently added test/Transforms/LoopStrengthReduce/RISCV/lsr-cost-compare.ll. Without a use outside the loop (see test3), we convert the IV into a pointer increment. With one, we leave the gep in place.

The pointer increment version both decreases number of instructions in some loops, and creates parallel chains of computation (i.e. decreases critical path depth). Both are generally profitable.

Arguably, we should really be using a more sophisticated model here - such as e.g. using profile information or explicitly modeling parallelism gains. However, as a practical matter starting with the same mild hack that other targets have used seems reasonable.

Diff Detail

Event Timeline

reames created this revision.Jan 20 2023, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 8:49 AM
reames requested review of this revision.Jan 20 2023, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 8:49 AM
asb accepted this revision.Jan 24 2023, 8:03 AM

This LGTM (I'm not very familiar with these cost calculations though, so consider this a weak approval!).

It looks like we might want to consider, in a followup patch, also implementing isNumRegsMajorCostOfLSR to returning false (introduced in D89665)?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
1479

Maybe "Use instruction count rather than number of registers as the dominant cost." would more clearly express how this differs to the default implementation?

This revision is now accepted and ready to land.Jan 24 2023, 8:03 AM

It looks like we might want to consider, in a followup patch, also implementing isNumRegsMajorCostOfLSR to returning false (introduced in D89665)?

I looked at that, but only one target actually uses it. The actual use doesn't seem to match the naming; it seems to in practice mean "turn off chain detection". I don't think we want to do that.

Makes sense to me and the results I'm seeing are reasonable. (this is also a weak approval)

reames added inline comments.Jan 24 2023, 11:41 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
1479

This exact comment is used in multiple targets. It looks like a good candidate for some code restructuring, and I biased in terms of making it easily grep-able.

This revision was landed with ongoing or failed builds.Jan 24 2023, 11:43 AM
This revision was automatically updated to reflect the committed changes.