Skip to content

Conversation

c8ef
Copy link
Contributor

@c8ef c8ef commented Jun 11, 2024

close: #86301

@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jun 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 11, 2024

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-selectiondag

Author: None (c8ef)

Changes

close: #86301


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+15)
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 4fcbe08e4b2b9..0a78803357410 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -5236,6 +5236,21 @@ SDValue DAGCombiner::visitAVG(SDNode *N) {
     return DAG.getNode(ISD::SRL, DL, VT, X,
                        DAG.getShiftAmountConstant(1, VT, DL));
 
+  // fold avgu(zext(x), zext(y)) -> zext(avgu(x, y))
+  SDValue A;
+  SDValue B;
+  if (hasOperation(ISD::AVGFLOORU, VT) &&
+      sd_match(N, m_c_BinOp(ISD::AVGFLOORU, m_ZExt(m_Value(A)),
+                            m_ZExt(m_Value(B))))) {
+    SDValue AvgFloorU = DAG.getNode(ISD::AVGFLOORU, DL, A.getValueType(), A, B);
+    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, AvgFloorU);
+  }
+  if (hasOperation(ISD::AVGCEILU, VT) &&
+      sd_match(N, m_c_BinOp(ISD::AVGCEILU, m_ZExt(m_Value(A)),
+                            m_ZExt(m_Value(B))))) {
+    SDValue AvgCeilU = DAG.getNode(ISD::AVGCEILU, DL, A.getValueType(), A, B);
+    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, AvgCeilU);
+  }
   return SDValue();
 }
 

@dtcxzyw dtcxzyw requested review from RKSimon and topperc June 11, 2024 15:44
@RKSimon
Copy link
Collaborator

RKSimon commented Jun 11, 2024

For tests you can start with these: https://simd.godbolt.org/z/xfrWo8Tc7

@c8ef c8ef requested review from dtcxzyw and jayfoad June 12, 2024 04:05
dtcxzyw added a commit to dtcxzyw/llvm-codegen-benchmark that referenced this pull request Jun 12, 2024
Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cheers

@jayfoad
Copy link
Contributor

jayfoad commented Jun 12, 2024

Any plans to do avgs(sext(x), sext(y)) -> sext(avgs(x, y)) ?

@RKSimon RKSimon merged commit 0e346ee into llvm:main Jun 12, 2024
@RKSimon
Copy link
Collaborator

RKSimon commented Jun 12, 2024

Any plans to do avgs(sext(x), sext(y)) -> sext(avgs(x, y)) ?

IIRC the constraints on that pattern aren't as easy as the unsigned cases.

@jayfoad
Copy link
Contributor

jayfoad commented Jun 12, 2024

Any plans to do avgs(sext(x), sext(y)) -> sext(avgs(x, y)) ?

IIRC the constraints on that pattern aren't as easy as the unsigned cases.

Really? My instinct is that it should Just Work, but I have not thought about it deeply.

@c8ef c8ef deleted the dag branch June 12, 2024 12:06
@RKSimon
Copy link
Collaborator

RKSimon commented Jun 12, 2024

Any plans to do avgs(sext(x), sext(y)) -> sext(avgs(x, y)) ?

IIRC the constraints on that pattern aren't as easy as the unsigned cases.

Really? My instinct is that it should Just Work, but I have not thought about it deeply.

I'm miss-remembering - it looks like it just needs an explicit freeze in the alive test where the unsigned cases didn't https://alive2.llvm.org/ce/z/qgp7bF

@c8ef
Copy link
Contributor Author

c8ef commented Jun 12, 2024

I would like to continue working on the signed case. Do we need to explicitly use SelectionDAG::getFreeze for the signed extended source value to match the alive test?

@dtcxzyw
Copy link
Member

dtcxzyw commented Jun 12, 2024

Any plans to do avgs(sext(x), sext(y)) -> sext(avgs(x, y)) ?

IIRC the constraints on that pattern aren't as easy as the unsigned cases.

Really? My instinct is that it should Just Work, but I have not thought about it deeply.

I'm miss-remembering - it looks like it just needs an explicit freeze in the alive test where the unsigned cases didn't https://alive2.llvm.org/ce/z/qgp7bF

I don't think we need freeze here because this fold doesn't introduce multiple uses as xor + or does. BTW, we already gave up on correct undef semantics in #90097.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:AArch64 llvm:SelectionDAG SelectionDAGISel as well

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DAG] Fold AVGU(ZEXT(X),ZEXT(Y)) -> ZEXT(AVGU(X,Y))

5 participants