Page MenuHomePhabricator

[RISCV] Add isel-patterns to optimize (a < 1) into blez (a <= 0)
ClosedPublic

Authored by philipp.tomsich on Mar 11 2021, 12:43 PM.

Details

Summary
The following code-sequence showed up in a testcase (isolated from
SPEC2017) for if-conversion and vectorization when searching for the
maximum in an array:
        addi    a2, zero, 1
        blt     a1, a2, .LBB0_5
which can be expressed as `bge zero,a1,.LBB0_5`/`blez a1,/LBB0_5`.

More generally, we want to express (a < 1) as (a <= 0).

This adds the required isel-pattern and updates the testcases.

Diff Detail

Event Timeline

philipp.tomsich requested review of this revision.Mar 11 2021, 12:43 PM
craig.topper added inline comments.Mar 11 2021, 12:54 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
996

We already have this in this exact spot in the file. Is your repo out of date?

// Match X > -1, the canonical form of X >= 0, to the bgez pattern.                                                                                                                                                                                                                                                                                                                                                                                                      
def : Pat<(brcond (XLenVT (setgt GPR:$rs1, -1)), bb:$imm12),                                                                                                                                                                                                                                                                                                                                                                                                             
          (BGE GPR:$rs1, X0, bb:$imm12)>;
philipp.tomsich retitled this revision from [RISCV] Add isel-patterns to optimize (a < 1) and (a > -1) into bge to [RISCV] Add isel-patterns to optimize (a < 1) into bgez.
philipp.tomsich edited the summary of this revision. (Show Details)

Added a testcase and rebased onto

commit 49942c6d4a0aee740381f754a6a0e6f7e1bfed43
Author: Quentin Colombet <qcolombet@apple.com>
Date:   Wed Mar 10 13:28:53 2021 -0800

This feels like it should be a target-independent DAGCombine?

This feels like it should be a target-independent DAGCombine?

DAG combine and instcombine canonicalize to constants on the RHS and SETGT, SETLT, SETUGT, and SETULT. So X <= 0 will always be rewritten to X< 1.

jrtc27 accepted this revision.Mar 12 2021, 8:24 AM

This feels like it should be a target-independent DAGCombine?

DAG combine and instcombine canonicalize to constants on the RHS and SETGT, SETLT, SETUGT, and SETULT. So X <= 0 will always be rewritten to X< 1.

Ah ok then this looks good to me. Would be good to have the test pre-committed with the worse codegen as usual though.

This revision is now accepted and ready to land.Mar 12 2021, 8:24 AM
frasercrmck added inline comments.Mar 12 2021, 8:28 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1001

Indentation looks a little off here. Everything else seems to have aligned the to and from.

craig.topper added inline comments.Mar 12 2021, 11:30 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
999

Isn't this the blez pattern?

The "into" doesn't read right after the "as"

Maybe
" Lower (a < 1) as (X0 >= a), the blez pattern."

llvm/test/CodeGen/RISCV/branch.ll
128

This code is pretty trivially unreachable. The block above jumps over it. I'm a little surprised CodeGenPrepare or UnreachableBlockElim didn't notice that. I'd prefer we make it reachable so it can't break in the future.

craig.topper requested changes to this revision.Mar 12 2021, 11:30 AM
This revision now requires changes to proceed.Mar 12 2021, 11:30 AM

FYI, I put up a related patch to fix this same issue for select https://reviews.llvm.org/D98542

craig.topper added inline comments.Mar 12 2021, 11:45 AM
llvm/test/CodeGen/RISCV/branch.ll
128

I applied this patch and this test case fails because the unreachable block does get deleted.

This patch also appears to affect llvm/test/CodeGen/RISCV/hoist-global-addr-base.ll

philipp.tomsich marked an inline comment as done.
philipp.tomsich retitled this revision from [RISCV] Add isel-patterns to optimize (a < 1) into bgez to [RISCV] Add isel-patterns to optimize (a < 1) into blez (a <= 0).
philipp.tomsich edited the summary of this revision. (Show Details)
philipp.tomsich removed a reviewer: jrtc27.

This updates the test cases and I have rerun 'ninja check' to ensure no further test issues.
The commit message has also been updated to reflect that this is a blez-instruction.

Update whitespaces to all-spaces for tabs-and-spaces.

Update the comment to reflect that this will turn into a blez instruction.

philipp.tomsich marked 4 inline comments as done.Mar 15 2021, 7:57 AM
Harbormaster completed remote builds in B93816: Diff 330660.
craig.topper accepted this revision.EditedMar 15 2021, 10:02 AM

LGTM. Do you need me to commit it? If so can you let me know your email and how you want the Author line to appear in the commit log.

This revision is now accepted and ready to land.Mar 15 2021, 10:02 AM

Please go ahead and commit this for me.

Thanks,
Philipp.

mhorne removed a subscriber: mhorne.Mar 15 2021, 10:55 AM