This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Override TargetLowering::BuildSDIVPow2 to generate SELECT
ClosedPublic

Authored by pcwang-thead on Dec 1 2021, 1:49 AM.

Details

Summary

When Zbt is enabled, we can generate SELECT for division by power
of 2, so that there is no data dependency.

Diff Detail

Event Timeline

pcwang-thead requested review of this revision.Dec 1 2021, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2021, 1:49 AM
craig.topper added inline comments.Dec 7 2021, 9:56 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10095

Capitalize 'only'

10105

Is i64 correct here for RV32? I don't see a test case.

10106

Use APInt::isNegatedPowerOf2

llvm/test/CodeGen/RISCV/rv32zbt-div-pow2.ll
1

Can you add test cases for dividing by 2 and -2 as well? X86 does not use BuildSDIVPow2 for those cases so we should check those.

This should probably be disabled for minsize.

jrtc27 added inline comments.Dec 8 2021, 9:09 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10096

Should say *why*, ie need conditional move

10100

X86 prioritises isIntDivCheap over a lack of conditional move; what's the justification for doing it the other way round?

10106

X86 asserts it's one of the two, so this check seems bogus given the function name...

llvm/test/CodeGen/RISCV/rv32zbt-div-pow2.ll
7

No positive power of 2 tests, and no tests for isIntDivCheap being true?

llvm/test/CodeGen/RISCV/rv64zbt-div-pow2.ll
8

Don't duplicate tests, just add rv64 check lines to your existing rv32-only test

31

And we should test we don't do stupid things for i64 on RV32

craig.topper added inline comments.Dec 9 2021, 11:56 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
10100

I agree with @jrtc27. The isIntDivCheap check should be at the top.

10106

I think AArch64 explicitly checks it. I wrote X86 later and used an assert.

llvm/test/CodeGen/RISCV/rv32zbt-div-pow2.ll
7

isIntDivCheap is never true for RISCV today.

We should also add a test for dividing by 4096 or larger for RV32 and RV64. And probably something larger than 2^32 for RV64. The constant materialization will add additional instructions not seen in the 1024 case.

This should probably be disabled for minsize.

That's what several targets use isIntDivCheap for. I don't think RISCV does. The patch here doesn't make that situation worse.

pcwang-thead updated this revision to Diff 393864.EditedDec 13 2021, 5:59 AM
  • Address comments.
  • Only apply to divisors whose absolute value is less than 2^12.

Some of the i32 ones look like they might not be profitable on RV64 unless you have a wide out-of-order processor; 50% more instructions in some cases

pcwang-thead marked 13 inline comments as done.Dec 13 2021, 6:32 AM

Some of the i32 ones look like they might not be profitable on RV64 unless you have a wide out-of-order processor; 50% more instructions in some cases

For i32 on RV64, sign extension is needed, so there are sext.w. And the only case with 50% more instructions is when divisor is 2.
Maybe we should skip these divisors, any ideas?

llvm/test/CodeGen/RISCV/rv32zbt-div-pow2.ll
7

isIntDivCheap is never true for RISCV today.

We should also add a test for dividing by 4096 or larger for RV32 and RV64. And probably something larger than 2^32 for RV64. The constant materialization will add additional instructions not seen in the 1024 case.

7

Thanks.

I added tests divided by n (|n| ≥ 2^12) and two materialization instructions were generated. So it may be non-profitable when absolute values of divisors are larger than 2^12.

craig.topper added inline comments.Dec 13 2021, 10:11 AM
llvm/test/CodeGen/RISCV/div-pow2.ll
14 ↗(On Diff #393864)

I think it makes sense to keep the old codegen when dividing by 2.

58 ↗(On Diff #393864)

Original code is less instructions and has the same critical path instruction count.

355 ↗(On Diff #393864)

Original code looks better.

406 ↗(On Diff #393864)

Original code looks better

pcwang-thead marked an inline comment as done.
  • Address comments.
pcwang-thead marked 4 inline comments as done.Dec 13 2021, 10:38 PM
jrtc27 added a comment.EditedDec 15 2021, 6:48 PM

From the LLVM Code-Review Policy and Practices (https://llvm.org/docs/CodeReview.html):

If it is urgent, provide reasons why it is important to you to get this patch landed and ping it every couple of days. If it is not urgent, the common courtesy ping rate is one week. Remember that you’re asking for valuable time from other professional developers.

It's only been 3 days for this and other patches you've just pinged

From the LLVM Code-Review Policy and Practices (https://llvm.org/docs/CodeReview.html):

If it is urgent, provide reasons why it is important to you to get this patch landed and ping it every couple of days. If it is not urgent, the common courtesy ping rate is one week. Remember that you’re asking for valuable time from other professional developers.

It's only been 3 days for this and other patches you've just pinged

Sorry, my bad. It's not so urgent.

This revision is now accepted and ready to land.Jan 7 2022, 9:40 AM
This revision was landed with ongoing or failed builds.Jan 10 2022, 11:55 PM
This revision was automatically updated to reflect the committed changes.