This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Combine comparison and logic ops.
ClosedPublic

Authored by iabg-sc on Sep 20 2022, 5:41 AM.

Details

Summary

Two comparison operations and a logical operation are combined into selection using MIN or MAX and comparison operation.
For optimization to be applied conditions have to be satisfied:

  1. In comparison operations has to be the one common operand.
  2. Supports only signed and unsigned integers.
  3. Comparison has to be the same with respect to common operand.
  4. There are no more users of comparison except logic operation.
  5. Every combination of comparison and AND, OR are supported.

It will convert

%l0 = %a < %c
%l1 = %b < %c
%res = %l0 or %l1

into

%sel = min(%a, %b)
%res = %sel < %c

It supports several comparison operations (<, <=, >, >=), signed, unsigned values and different order of operands if they do not violate conditions.

Diff Detail

Event Timeline

iabg-sc created this revision.Sep 20 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 5:41 AM
iabg-sc requested review of this revision.Sep 20 2022, 5:41 AM
iabg-sc updated this revision to Diff 461547.Sep 20 2022, 5:48 AM

Fix commit message

craig.topper added inline comments.
llvm/test/CodeGen/RISCV/zbb-cmp-combine.ll
6

oprimization -> optimization

241

I don't think this is correct for FP. "fcmp ult" returns true if either operand is a nan. fmin will return a non-nan input unless both are nan. So for this code, if %a is nan, %b and %c are non-nan the original code returned true. But the rewritten code will only compare %b and %c and ignore %a.

[test] normally means you’re only touching tests. The rest of the subject is also poor, you don’t have an actual active verb, just a participle

iabg-sc retitled this revision from [RISCV][test] Combining comparison and logic ops. #17574 to [RISCV] Combining comparison and logic ops..Sep 22 2022, 3:04 AM
iabg-sc updated this revision to Diff 462126.EditedSep 22 2022, 4:00 AM

Floating point operands are not supported. Typos fix.

iabg-sc retitled this revision from [RISCV] Combining comparison and logic ops. to [RISCV] Combine comparison and logic ops..Sep 22 2022, 4:03 AM
iabg-sc edited the summary of this revision. (Show Details)
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8507

please adjust comment: no float

iabg-sc updated this revision to Diff 462138.Sep 22 2022, 5:03 AM

'.' was removed from commit message

iabg-sc updated this revision to Diff 462141.Sep 22 2022, 5:06 AM

comment about float support was removed

iabg-sc added inline comments.Sep 27 2022, 3:40 AM
llvm/test/CodeGen/RISCV/zbb-cmp-combine.ll
241

Yes, indeed. Since the behavior with NaNs cannot be preserved these cases should be prohibited.

@asb, @craig.topper, @reames could you please review?

Hi!
Could you please review?

craig.topper added inline comments.Oct 12 2022, 10:19 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8418

Drop curly braces

8422

Drop curly braces

8426

Why do we need to get the underlying SDNode* explicitly? SDValue has a operator->.

8461

Does this get reported as unused in release builds?

8522

Drop curly braces

8538

All of the less conditions have bit 1 set and bit 2 clear. All the greater have bit 1 clear and bit 2 set. That's by design. Can we use that?

craig.topper added inline comments.Oct 12 2022, 10:20 PM
llvm/test/CodeGen/RISCV/zbb-cmp-combine.ll
11

below is misspelled

iabg-sc updated this revision to Diff 467480.Oct 13 2022, 8:19 AM

Comparison operations are checked via bit fields.
SDNodes were removed. Now all actions only on SDValues.
Some cosmetic changes.

Thanks for the suggestions. Here is the patch with fixes mentioned before.

craig.topper added inline comments.Oct 19 2022, 2:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8418

This is required by the definition of ISD::AND/OR. It can be an assert.

8429

I'm not sure the std::finds and std::distance added much over manually checking the 4 permutations.

8470

pattern*

8500

If the compares a common operand, then all of the operands must be the same type.

iabg-sc added inline comments.Oct 20 2022, 3:20 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8429

Perhaps searching for common and other operands is easy to do manually. It can be done with two ifs. But verifying the found operands will be look like copy-paste with different indices. Now this verifying is hidden inside algorithms, and only crucial conditions, when something goes terribly wrong, are checked with assert.
I think there are two different approaches:

1 - Find correct indices by value properties. Therefore, I do not need to check this properties as they are stated. This is that I did.
2 - By indices find values and verify their properties. But in this case indices are stated and addition checks are required.
iabg-sc updated this revision to Diff 469170.Oct 20 2022, 5:07 AM

Removed unnecessary checks. Fixed typos.

Hi!
Could you please review?

iabg-sc updated this revision to Diff 474527.Nov 10 2022, 6:01 AM

Rebased to upstream

Hi!
Could you please review?

iabg-sc updated this revision to Diff 482436.Dec 13 2022, 5:04 AM

New approach

Implemented new approach to determine common operands in comparison operations.

Hi!
Could you please review?

craig.topper added inline comments.Dec 13 2022, 1:52 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8508

Why at instead of [Pos]?

8514

std::optional

8519

std::nullopt

8551

std::optional

8563

std::nullopt

8566

V0 and V1 are SDValue. We should use .getOpcode() instead of ->getOpcode().

8570

.getOperand(0)

8628

I'm not sure the enum makes sense if you have to assert that it isn't once of the values. Can't we just have a bool IsUnsigned?

8640

LLVM is pretty conservative about the use of auto. Use unsigned here.

8650

use EVT instead of auto.

8654

Seems like rather than an enum we can have

bool IsUnsigned = ISD::isUnsignedIntSetCC(RefOpcode);
assert((IsUnsigned || ISD::isSignedIntSetCC(RefOpcode)) &&
       "Operation neither with signed or unsigned integers.");
``
craig.topper added inline comments.Dec 13 2022, 1:59 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8466

possition -> position

craig.topper added inline comments.Dec 13 2022, 2:04 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8578

Why std::move? There's nothing in CmpOpInfo that is heap allocated is there?

iabg-sc updated this revision to Diff 482762.Dec 14 2022, 1:45 AM

Minor changes for code readability.

iabg-sc added inline comments.Dec 14 2022, 1:50 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8654

Thanks. It is more clear. Initially enum was used, because there could be fp operations too.

Thanks for the review. Could you please look on this version?

Hi!
Could you please review?

This revision is now accepted and ready to land.Dec 22 2022, 10:34 AM
This revision was landed with ongoing or failed builds.Dec 23 2022, 6:11 AM
This revision was automatically updated to reflect the committed changes.