Added v2i32 test.

Moved VT check right into function.

Factored out reassociateScalarOps().

Removed flags.
Added gfx9 run line.

LGTM

LGTM

In D55301#1393065, @arsenm wrote:

ping

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

Addressed review comments.

LGTM

LGTM

LGTM

Can you add a test where low half does not produce a chain? An arithmetic operation and an undef.

Removed call to ShrinkDemandedConstant completely.

In D56406#1349338, @craig.topper wrote:

Doesn't SimplifyDemandedBits already call ShrinkDemandedConstant for AND/OR/XOR? And without an override of targetShrinkDemandedConstant, ShrinkDemandedConstant only works on those 3 nodes. So is an explicit call to ShrinkDemandedConstant even needed here?

In D56406#1349488, @arsenm wrote:

Something seems wrong here. How/why is this valid on AArch64 in some situation? A test is really needed for whatever this regression is

In D56289#1348541, @rampitec wrote:

In D56289#1348529, @rampitec wrote:

In D56289#1348172, @fhahn wrote:

I am seeing a regression with this change on AArch64, where we do not use shrinked constants, e.g. for and/or if they have multiple users. I am not entirely sure what the problem is this patch is solving, as I am not familiar with AMDGPU. Is it a case where shrinking the constant leads to shrinking the related Op and in the end this introduces unnecessary conversion code, as multiple users require different bitwidths?

If that is the case, I am not sure if this is the best place to fix the issue. IIUC, shrinkDemandedBits just shrinks the constant of the passed in Op, but not the width of the result type of Op. So it seems to me that shrinking the constant should be independent of the users of Op.

Here is DAG exposing the problem:

t33: i32 = or t42, Constant:i32<-2147483647>
        t37: f32 = bitcast t33
        t66: f32 = CVT_F32_UBYTE0 t33
      t38: f32 = fadd t37, t66

The node t66 only needs a low byte from the t33 "or" node. The other use of "or" is bitcast and subsequently t38 fadd which needs full dword.
The "Op" argument of ShrinkDemandedConstant() is t33 "or" and Demanded = 0xff as needed for t66.

If optimization succeeds it will replace "or t42, 0x80000001" with "or t42, 1". However, this "or" has another use which needs the full range of the value.
That is a bug not to take other uses into account.

It is unfortunate this lead to regression on AArch64 side. Are you sure a similar issue can never happen with AArch64?
Theoretically that is possible to move a single use check past targetShrinkDemandedConstant() call which actually do the job for AArch I presume.
I think it should correct lack of optimization in that specific case, but I am not sure there will be no correctness issues elsewhere. Here is what I am speaking about:

--- a/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -350,13 +350,13 @@ bool TargetLowering::ShrinkDemandedConstant(SDValue Op, const APInt &Demanded,
   SDLoc DL(Op);
   unsigned Opcode = Op.getOpcode();

-  if (!Op.hasOneUse())
-    return false;
-
   // Do target-specific constant optimization.
   if (targetShrinkDemandedConstant(Op, Demanded, TLO))
     return TLO.New.getNode();

+  if (!Op.hasOneUse())
+    return false;
+
   // FIXME: ISD::SELECT, ISD::SELECT_CC
   switch (Opcode) {
   default:

In fact the bit width of all operations is the same. It just t66 only known to read low byte from its operand. I believe it worth nothing t66 is custom AMDGPU node.
Consider t66 is a shift left by 24:

t33: i32 = or t42, Constant:i32<-2147483647>
        t66: i32 = shl t33, 24
        t38: i32 = add t33, t66

There is nothing target specific here. t66 also only known to read low 8 bits of t33 and that is not unreasonable to call ShrinkDemandedConstant on t33 with Demanded = 0xff. It will expose the same bug regardless of target.

In D56289#1348529, @rampitec wrote:

In D56289#1348172, @fhahn wrote:

I am seeing a regression with this change on AArch64, where we do not use shrinked constants, e.g. for and/or if they have multiple users. I am not entirely sure what the problem is this patch is solving, as I am not familiar with AMDGPU. Is it a case where shrinking the constant leads to shrinking the related Op and in the end this introduces unnecessary conversion code, as multiple users require different bitwidths?

If that is the case, I am not sure if this is the best place to fix the issue. IIUC, shrinkDemandedBits just shrinks the constant of the passed in Op, but not the width of the result type of Op. So it seems to me that shrinking the constant should be independent of the users of Op.

Here is DAG exposing the problem:

t33: i32 = or t42, Constant:i32<-2147483647>
        t37: f32 = bitcast t33
        t66: f32 = CVT_F32_UBYTE0 t33
      t38: f32 = fadd t37, t66

The node t66 only needs a low byte from the t33 "or" node. The other use of "or" is bitcast and subsequently t38 fadd which needs full dword.
The "Op" argument of ShrinkDemandedConstant() is t33 "or" and Demanded = 0xff as needed for t66.

If optimization succeeds it will replace "or t42, 0x80000001" with "or t42, 1". However, this "or" has another use which needs the full range of the value.
That is a bug not to take other uses into account.

It is unfortunate this lead to regression on AArch64 side. Are you sure a similar issue can never happen with AArch64?
Theoretically that is possible to move a single use check past targetShrinkDemandedConstant() call which actually do the job for AArch I presume.
I think it should correct lack of optimization in that specific case, but I am not sure there will be no correctness issues elsewhere. Here is what I am speaking about:

--- a/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -350,13 +350,13 @@ bool TargetLowering::ShrinkDemandedConstant(SDValue Op, const APInt &Demanded,
   SDLoc DL(Op);
   unsigned Opcode = Op.getOpcode();

-  if (!Op.hasOneUse())
-    return false;
-
   // Do target-specific constant optimization.
   if (targetShrinkDemandedConstant(Op, Demanded, TLO))
     return TLO.New.getNode();

+  if (!Op.hasOneUse())
+    return false;
+
   // FIXME: ISD::SELECT, ISD::SELECT_CC
   switch (Opcode) {
   default:

In D56289#1348172, @fhahn wrote:

I am seeing a regression with this change on AArch64, where we do not use shrinked constants, e.g. for and/or if they have multiple users. I am not entirely sure what the problem is this patch is solving, as I am not familiar with AMDGPU. Is it a case where shrinking the constant leads to shrinking the related Op and in the end this introduces unnecessary conversion code, as multiple users require different bitwidths?

If that is the case, I am not sure if this is the best place to fix the issue. IIUC, shrinkDemandedBits just shrinks the constant of the passed in Op, but not the width of the result type of Op. So it seems to me that shrinking the constant should be independent of the users of Op.

LGTM

Moved single use check into ShrinkDemandedConstant.

In D56289#1345663, @arsenm wrote:

Probably should try fixing ShrinkDemandedConstant first

In D56289#1345663, @arsenm wrote:

Probably should try fixing ShrinkDemandedConstant first

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

It looks good to me, but @arsenm could you also take a look?

LGTM

LGTM

LGTM

LGTM

LGTM