Page MenuHomePhabricator

[RISCV] Combine comparison and logic ops.
Needs ReviewPublic

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
8303

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
8214

Drop curly braces

8218

Drop curly braces

8222

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

8257

Does this get reported as unused in release builds?

8318

Drop curly braces

8334

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
8214

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

8225

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

8266

pattern*

8296

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
8225

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.Thu, Nov 10, 6:01 AM

Rebased to upstream

Hi!
Could you please review?