[RISCV] Fold binary op into select if profitable.

[mgudim]

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5527

dyn_cast->cast if it can't fail

5527

const APInt &

[mgudim]

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]

Pre-merge checks passed.

[craig.topper]

LGTM

[mgudim]

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?

[jrtc27]

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).

[craig.topper]

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.

[craig.topper]

I have reverted the change f9d0bf06319203a8cbb47d89c2f39d2c782f3887

[mgudim]

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]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5700

Please upload patches with full context using git diff -U99999

[mgudim]

Uploaded diff with -u99999 for more context.

[mgudim]

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]

(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]

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

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5823

Use a plain old C array?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5808

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

[mgudim]

Addressed comments: use plain array instead of SmallVector

[mgudim]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5808

Makes sence. Done.

[mgudim]

@craig.topper is it ok to recommit this?

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5789

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

5968

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]

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]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5968

Thanks, I never thought about that.

[craig.topper]

LGTM

[mgudim]

rebased on current main.

[mgudim]

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

[craig.topper]

In D152147#4502074, @mgudim wrote:

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.