This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fold binary op into select if profitable.
ClosedPublic

Authored by mgudim on Jun 5 2023, 5:45 AM.

Details

Summary

Consider the following pattern binOp (select cond, x, c0), c1.
Where c0 and c1 are constants.
We can transform it to select cond, binOp(x, c1), binOp(c0, c1).

If binOp(c0, c1) ends up being 0 or -1 we can turn the select into
a more profitable sequence.

Diff Detail

Event Timeline

mgudim created this revision.Jun 5 2023, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 5:45 AM
mgudim requested review of this revision.Jun 5 2023, 5:45 AM
craig.topper added inline comments.Jun 5 2023, 3:54 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5527

dyn_cast->cast if it can't fail

5527

const APInt &

mgudim updated this revision to Diff 529033.Jun 6 2023, 1:53 PM

Addressed comments.
Fixed a bug: the constants were passed to DAG.FoldConstantArithmetic in the wrong order.
Moved call to foldBinOpIntoSelectIfProfitable after combineSelectToBinOp - this way we will not fold binop if the constant of select is already 0 or -1.
Minor typo fixes.

mgudim marked 2 inline comments as done.Jun 6 2023, 1:54 PM
mgudim updated this revision to Diff 529040.Jun 6 2023, 2:03 PM
mgudim updated this revision to Diff 529261.Jun 7 2023, 5:10 AM

Pre-merge checks passed.

This revision is now accepted and ready to land.Jun 8 2023, 11:34 AM

Thanks @craig.topper , I'll merge it (once I figure out the process). What about https://reviews.llvm.org/D151750 Can it be merged after this one?

This was committed as d0189584631e587279ee5f0af5feb94d8045bb31 without the normal Phab metadata (i.e. without arc patch), but is causing assertion failures building the Linux kernel per https://github.com/llvm/llvm-project/issues/63293.

Please revert this, debug the issue and re-land with an appropriate fix and the correct metadata per the LLVM Phabricator documentation (https://llvm.org/docs/Phabricator.html#committing-a-change).

jrtc27 requested changes to this revision.Jun 13 2023, 4:47 PM
This revision now requires changes to proceed.Jun 13 2023, 4:47 PM

Please revert. I think the issue is that it never checks the opcode of the binary operator which might not even be a binary operator. It might not even be something that FoldConstantArithmetic can handle.

mgudim updated this revision to Diff 531527.Jun 14 2023, 3:04 PM

added logic for the correct order of operands when creating new opepands of select.
added test for all four possible positions of constants in select and binop.

craig.topper added inline comments.Jun 14 2023, 3:07 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5700

Please upload patches with full context using git diff -U99999

mgudim updated this revision to Diff 531795.Jun 15 2023, 9:38 AM

Uploaded diff with -u99999 for more context.

mgudim marked an inline comment as done.Jun 15 2023, 9:39 AM
mgudim updated this revision to Diff 533012.Jun 20 2023, 12:03 PM
mgudim updated this revision to Diff 533575.Jun 22 2023, 5:54 AM

Can't reproduce the failed tests locally. Put a print statement in the code to make sure it is the most recent patch being tested.

mgudim updated this revision to Diff 538821.Jul 10 2023, 2:24 PM

(1) Added a check if FoldConstantArithmetic returns null.
(2) Fixed the incorrect use of ArrayRef. This was causing a bug, which is not visible when compiled in Debug mode.

mgudim updated this revision to Diff 539079.Jul 11 2023, 7:10 AM

Changed ArrayRef to SmallVector in two more lines, which I missed in the previous update.

craig.topper added inline comments.Jul 11 2023, 8:58 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5818

Use a plain old C array?

craig.topper added inline comments.Jul 11 2023, 10:23 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5803

You can use a plain C array and std::swap(NewConstOps[0], NewConstOps[1]) if SelOpNo == 1.

mgudim updated this revision to Diff 539196.Jul 11 2023, 10:55 AM

Addressed comments: use plain array instead of SmallVector

mgudim marked 2 inline comments as done.Jul 11 2023, 10:56 AM
mgudim added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5803

Makes sence. Done.

mgudim marked an inline comment as done.Jul 13 2023, 7:00 AM

@craig.topper is it ok to recommit this?

craig.topper added inline comments.Jul 13 2023, 10:18 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5784

Can ConstSelOp and ConstSelOpNode be moved below the if by using ConstSelOpNo?

5846

Is this safe for division/remainder? The select could be sanitizing inputs. RISC-V doesn't have traps for div/rem, but IR/SelectionDAG semantics consider division by 0 to be immediate UB. Same with (sdiv -1, INT_MIN)

mgudim updated this revision to Diff 540466.Jul 14 2023, 9:58 AM

addressed comments:
(1) Moved definitions of ConstSelOp and ConstSelOpNode lower.
(2) Added check DAG.isSafeToSpeculativelyExecute(...) to check if commuting select and binop is legal.

mgudim marked 2 inline comments as done.Jul 14 2023, 9:58 AM
mgudim added inline comments.Jul 14 2023, 9:59 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5846

Thanks, I never thought about that.

mgudim updated this revision to Diff 540490.Jul 14 2023, 10:39 AM

rebased on current main.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 14 2023, 12:31 PM
This revision was automatically updated to reflect the committed changes.

I have accidentally commited this in "Needs Review" state. What should I do?

I have accidentally commited this in "Needs Review" state. What should I do?

It was only in Needs Review due to @jrtc27's. I believe those comments were addressed. The new commit has the phabricator metadata. I think you're ok unless Jessica speaks up.