[InstCombine] Resolve TODO: Remove one-time check if other logic operand (Y) is constant #77973

AZero13 · 2024-01-12T20:17:28Z

By using match(W, m_ImmConstant()), we do not need to worry about one-time use anymore.

llvmbot · 2024-01-12T20:17:58Z

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

By using isa<Constant>(W), we do not need to worry about one-time use anymore.

Full diff: https://github.com/llvm/llvm-project/pull/77973.diff

1 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp (+8-4)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index b7958978c450c9..8614aad529eb4a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -368,10 +368,14 @@ static Instruction *foldShiftOfShiftedBinOp(BinaryOperator &I,
 
   // Find a matching one-use shift by constant. The fold is not valid if the sum
   // of the shift values equals or exceeds bitwidth.
-  // TODO: Remove the one-use check if the other logic operand (Y) is constant.
   Value *X, *Y;
-  auto matchFirstShift = [&](Value *V) {
+  auto matchFirstShift = [&](Value *V, Value *W) {
     APInt Threshold(Ty->getScalarSizeInBits(), Ty->getScalarSizeInBits());
+    if (isa<Constant>(W)) {
+      return match(V, (m_BinOp(ShiftOpcode, m_Value(X), m_Constant(C0)))) &&
+             match(ConstantExpr::getAdd(C0, C1),
+                   m_SpecificInt_ICMP(ICmpInst::ICMP_ULT, Threshold));
+    }
     return match(V,
                  m_OneUse(m_BinOp(ShiftOpcode, m_Value(X), m_Constant(C0)))) &&
            match(ConstantExpr::getAdd(C0, C1),
@@ -382,9 +386,9 @@ static Instruction *foldShiftOfShiftedBinOp(BinaryOperator &I,
   // is not so we cannot reoder if we match operand(1) and need to keep the
   // operands in their original positions.
   bool FirstShiftIsOp1 = false;
-  if (matchFirstShift(BinInst->getOperand(0)))
+  if (matchFirstShift(BinInst->getOperand(0), BinInst->getOperand(1)))
     Y = BinInst->getOperand(1);
-  else if (matchFirstShift(BinInst->getOperand(1))) {
+  else if (matchFirstShift(BinInst->getOperand(1), BinInst->getOperand(0))) {
     Y = BinInst->getOperand(0);
     FirstShiftIsOp1 = BinInst->getOpcode() == Instruction::Sub;
   } else

nikic

Tests?

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp

AZero13 · 2024-01-13T21:54:11Z

Tests?

Sure. I added a whole bunch.

llvm/test/Transforms/InstCombine/shift-logic.ll

AZero13 · 2024-01-16T21:44:49Z

@nikic Alright. I think I got this right. Though I am concerned about the shl undef tests that seem to fold things into poison values. I do not know if this is what we want.

AZero13 · 2024-01-21T19:55:00Z

@nikic Can I use the other tests that I removed but is affected by the transform then?

nikic · 2024-01-21T20:22:45Z

You need the simplest possible test (no vectors...) that does not fold before your changes and folds after your changes. Please find this test on your own.

AZero13 · 2024-01-21T21:18:18Z

You need the simplest possible test (no vectors...) that does not fold before your changes and folds after your changes. Please find this test on your own.

Did. @nikic Is this good?

jrtc27 · 2024-01-21T21:21:32Z

llvm/test/Transforms/InstCombine/shift-logic.ll

@@ -433,9 +433,9 @@ define i8 @shl_sub(i8 %x, i8 %y) {
 define i8 @shl_sub_multiuse(i8 %x) {
 ; CHECK-LABEL: @shl_sub_multiuse(
 ; CHECK-NEXT:    [[SH0:%.*]] = shl i8 [[X:%.*]], 3
-; CHECK-NEXT:    [[R:%.*]] = sub i8 [[SH0]], 42 ; constant operand on the 'sub'


Uh, did update_llc_test_checks.py really add that comment?...

It removed the comment, not added it @nikic

It added the comment as it was in the original IR, but when it was transformed llc removed it.

I know, but why was it there in the first place. The previous commit added it, but that suggests the first commit is wrong.

And please do not resolve conversations off the bat like that.

Because I put it there originally.

Well don't then?

And for the second time, stop resolving conversations like that.

AZero13 · 2024-01-21T22:40:59Z

Alright @nikic Can we please merge this?

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp

llvm/test/Transforms/InstCombine/shift-logic.ll

AZero13 · 2024-01-22T18:56:24Z

@nikic Done!

…tant By using match(W, m_ImmConstant()), we do not need to worry about one-time use anymore.

nikic

LGTM

LLVMCoCCommittee · 2024-04-26T23:01:36Z

@AtariDreams We have reached out to request your feedback via the email attached to your GitHub account. Please respond to [email protected]

AZero13 requested a review from nikic as a code owner January 12, 2024 20:17

llvmbot added the llvm:transforms label Jan 12, 2024

AZero13 force-pushed the puts branch from 53cfb91 to 265db14 Compare January 12, 2024 20:17

AZero13 changed the title ~~[Transforms] Remove one-time check if other logic operand (Y) is constant.~~ [Transforms] Remove one-time check if other logic operand (Y) is constant Jan 12, 2024

nikic changed the title ~~[Transforms] Remove one-time check if other logic operand (Y) is constant~~ [InstCombine] Remove one-time check if other logic operand (Y) is constant Jan 12, 2024

nikic reviewed Jan 12, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp Outdated Show resolved Hide resolved

AZero13 force-pushed the puts branch 3 times, most recently from cfc03de to 8f9d367 Compare January 13, 2024 03:13

AZero13 changed the title ~~[InstCombine] Remove one-time check if other logic operand (Y) is constant~~ [InstCombine] Resolve FIXME: Remove one-time check if other logic operand (Y) is constant Jan 13, 2024

AZero13 changed the title ~~[InstCombine] Resolve FIXME: Remove one-time check if other logic operand (Y) is constant~~ [InstCombine] Resolve TODO: Remove one-time check if other logic operand (Y) is constant Jan 13, 2024

AZero13 force-pushed the puts branch 2 times, most recently from 67361d8 to a9f84d5 Compare January 13, 2024 21:53

AZero13 requested a review from nikic January 13, 2024 21:54

AZero13 force-pushed the puts branch 4 times, most recently from 1726104 to 836c67d Compare January 15, 2024 00:30

nikic reviewed Jan 15, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/shift-logic.ll Outdated Show resolved Hide resolved

AZero13 force-pushed the puts branch 2 times, most recently from 15bf60c to ea3ee13 Compare January 16, 2024 03:14

nikic reviewed Jan 16, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/shift-logic.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/shift-logic.ll Outdated Show resolved Hide resolved

AZero13 force-pushed the puts branch 2 times, most recently from f68212c to 77f7092 Compare January 16, 2024 21:44

AZero13 requested a review from nikic January 16, 2024 21:47

AZero13 force-pushed the puts branch from 77f7092 to f25e726 Compare January 17, 2024 02:05

AZero13 requested a review from nikic January 21, 2024 15:41

AZero13 force-pushed the puts branch from 10b2f49 to 3019567 Compare January 21, 2024 21:08

AZero13 force-pushed the puts branch from 3019567 to 4dd204e Compare January 21, 2024 21:20

jrtc27 reviewed Jan 21, 2024

View reviewed changes

AZero13 force-pushed the puts branch from 4dd204e to 20b8d9b Compare January 21, 2024 21:32

topperc reviewed Jan 22, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp Outdated Show resolved Hide resolved

AZero13 force-pushed the puts branch from 20b8d9b to d6c39e8 Compare January 22, 2024 04:01

AZero13 requested a review from topperc January 22, 2024 04:04

AZero13 force-pushed the puts branch 4 times, most recently from abb1753 to 314fa05 Compare January 22, 2024 05:35

nikic reviewed Jan 22, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/shift-logic.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/shift-logic.ll Outdated Show resolved Hide resolved

AZero13 force-pushed the puts branch 2 times, most recently from 90def1e to b235e03 Compare January 22, 2024 18:56

AZero13 requested a review from nikic January 22, 2024 18:56

AZero13 added 2 commits January 22, 2024 19:53

[InstCombine] Add pre-commit tests [NFC]

c2d21f3

[Transforms] Remove one-time check if other logic operand (Y) is cons…

9fdbd5a

…tant By using match(W, m_ImmConstant()), we do not need to worry about one-time use anymore.

AZero13 force-pushed the puts branch from b235e03 to 9fdbd5a Compare January 23, 2024 00:54

nikic approved these changes Jan 23, 2024

View reviewed changes

nikic merged commit 96adf69 into llvm:main Jan 23, 2024

AZero13 deleted the puts branch January 23, 2024 13:05

[InstCombine] Resolve TODO: Remove one-time check if other logic operand (Y) is constant #77973

[InstCombine] Resolve TODO: Remove one-time check if other logic operand (Y) is constant #77973

Uh oh!

Conversation

AZero13 commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 12, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AZero13 commented Jan 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AZero13 commented Jan 16, 2024

Uh oh!

AZero13 commented Jan 21, 2024

Uh oh!

nikic commented Jan 21, 2024

Uh oh!

AZero13 commented Jan 21, 2024

Uh oh!

jrtc27 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

AZero13 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

AZero13 Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrtc27 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

AZero13 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

jrtc27 Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

AZero13 commented Jan 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AZero13 commented Jan 22, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

LLVMCoCCommittee commented Apr 26, 2024

Uh oh!

Uh oh!

AZero13 commented Jan 12, 2024 •

edited

Loading

AZero13 Jan 21, 2024 •

edited

Loading