Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns #86222

houndlord · 2024-03-22T00:23:20Z

github-actions · 2024-03-22T00:23:40Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-03-22T00:24:12Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-selectiondag

Author: None (houndlord)

Changes

Fixes [#8577 ].

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

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+42)
(modified) llvm/test/CodeGen/AArch64/hadd-combine.ll (+24)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 7009f375df1151..8debff80628432 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -2546,6 +2546,23 @@ static SDValue combineFixedwidthToAVGCEILU(SDNode *N, SelectionDAG &DAG) {
   return SDValue();
 }
 
+// Attempt to form avgceils(A, B) from (A | B) - ((A ^ B) >> 1)
+static SDValue combineFixedwidthToAVGCEILS(SDNode *N, SelectionDAG &DAG) {
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  SDValue N0 = N->getOperand(0);
+  EVT VT = N0.getValueType();
+  SDLoc DL(N);
+  if (TLI.isOperationLegal(ISD::AVGCEILS, VT)) {
+    SDValue A, B;
+    if (sd_match(N, m_Sub(m_Or(m_Value(A), m_Value(B)),
+                          m_Sra(m_Xor(m_Deferred(A), m_Deferred(B)),
+                                m_SpecificInt(1))))) {
+      return DAG.getNode(ISD::AVGCEILS, DL, VT, A, B);
+    }
+  }
+  return SDValue();
+}
+
 /// Try to fold a 'not' shifted sign-bit with add/sub with constant operand into
 /// a shift and add with a different constant.
 static SDValue foldAddSubOfSignBit(SDNode *N, SelectionDAG &DAG) {
@@ -2854,6 +2871,23 @@ static SDValue combineFixedwidthToAVGFLOORU(SDNode *N, SelectionDAG &DAG) {
   return SDValue();
 }
 
+// Attempt to form avgfloors(A, B) from (A & B) + ((A ^ B) >> 1)
+static SDValue combineFixedwidthToAVGFLOORS(SDNode *N, SelectionDAG &DAG) {
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  SDValue N0 = N->getOperand(0);
+  EVT VT = N0.getValueType();
+  SDLoc DL(N);
+  if (TLI.isOperationLegal(ISD::AVGFLOORS, VT)) {
+    SDValue A, B;
+    if (sd_match(N, m_Add(m_And(m_Value(A), m_Value(B)),
+                          m_Sra(m_Xor(m_Deferred(A), m_Deferred(B)),
+                                m_SpecificInt(1))))) {
+      return DAG.getNode(ISD::AVGFLOORS, DL, VT, A, B);
+    }
+  }
+  return SDValue();
+}
+
 SDValue DAGCombiner::visitADD(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
@@ -2872,6 +2906,10 @@ SDValue DAGCombiner::visitADD(SDNode *N) {
   // Try to match AVGFLOORU fixedwidth pattern
   if (SDValue V = combineFixedwidthToAVGFLOORU(N, DAG))
     return V;
+  
+  // Try to match AVGFLOORU fixedwidth pattern
+  if (SDValue V = combineFixedwidthToAVGFLOORS(N, DAG))
+    return V;
 
   // fold (a+b) -> (a|b) iff a and b share no bits.
   if ((!LegalOperations || TLI.isOperationLegal(ISD::OR, VT)) &&
@@ -3867,6 +3905,10 @@ SDValue DAGCombiner::visitSUB(SDNode *N) {
   if (SDValue V = combineFixedwidthToAVGCEILU(N, DAG))
     return V;
 
+  // Try to match AVGCEILS fixedwidth pattern
+  if (SDValue V = combineFixedwidthToAVGCEILS(N, DAG))
+    return V;
+
   if (SDValue V = foldAddSubMasked1(false, N0, N1, DAG, SDLoc(N)))
     return V;
 
diff --git a/llvm/test/CodeGen/AArch64/hadd-combine.ll b/llvm/test/CodeGen/AArch64/hadd-combine.ll
index e12502980790da..cbc0386ea8553a 100644
--- a/llvm/test/CodeGen/AArch64/hadd-combine.ll
+++ b/llvm/test/CodeGen/AArch64/hadd-combine.ll
@@ -341,6 +341,18 @@ define <8 x i16> @sub_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
   ret <8 x i16> %res
 }
 
+define <8 x i16> @shsub_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
+; CHECK-LABEL: shsub_fixedwidth_v4i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    srhadd v0.8h, v0.8h, v1.8h
+; CHECK-NEXT:    ret
+  %or = or <8 x i16> %a0, %a1
+  %xor = xor <8 x i16> %a0, %a1
+  %srl = ashr <8 x i16> %xor, <i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1>
+  %res = sub <8 x i16> %or, %srl
+  ret <8 x i16> %res
+}
+
 define <8 x i16> @rhaddu_base(<8 x i16> %src1, <8 x i16> %src2) {
 ; CHECK-LABEL: rhaddu_base:
 ; CHECK:       // %bb.0:
@@ -879,6 +891,18 @@ define <8 x i16> @uhadd_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
   ret <8 x i16> %res
 }
 
+define <8 x i16> @shadd_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
+; CHECK-LABEL: shadd_fixedwidth_v4i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    shadd v0.8h, v0.8h, v1.8h
+; CHECK-NEXT:    ret
+  %and = and <8 x i16> %a0, %a1
+  %xor = xor <8 x i16> %a0, %a1
+  %srl = ashr <8 x i16> %xor, <i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1>
+  %res = add <8 x i16> %and, %srl
+  ret <8 x i16> %res
+}
+
 declare <8 x i8> @llvm.aarch64.neon.shadd.v8i8(<8 x i8>, <8 x i8>)
 declare <4 x i16> @llvm.aarch64.neon.shadd.v4i16(<4 x i16>, <4 x i16>)
 declare <2 x i32> @llvm.aarch64.neon.shadd.v2i32(<2 x i32>, <2 x i32>)

github-actions · 2024-03-22T00:25:45Z

✅ With the latest revision this PR passed the C/C++ code formatter.

RKSimon · 2024-03-22T10:22:18Z

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

@@ -3867,6 +3905,10 @@ SDValue DAGCombiner::visitSUB(SDNode *N) {
  if (SDValue V = combineFixedwidthToAVGCEILU(N, DAG))
    return V;

+  // Try to match AVGCEILS fixedwidth pattern
+  if (SDValue V = combineFixedwidthToAVGCEILS(N, DAG))


Merge this into combineFixedwidthToAVGCEILU (rename to combineFixedwidthToAVGCEIL)

RKSimon · 2024-03-22T10:22:47Z

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

@@ -2872,6 +2906,10 @@ SDValue DAGCombiner::visitADD(SDNode *N) {
  // Try to match AVGFLOORU fixedwidth pattern
  if (SDValue V = combineFixedwidthToAVGFLOORU(N, DAG))
    return V;
+
+  // Try to match AVGFLOORU fixedwidth pattern
+  if (SDValue V = combineFixedwidthToAVGFLOORS(N, DAG))


Merge this into combineFixedwidthToAVGFLOORU (rename to combineFixedwidthToAVGFLOOR)

davemgreen · 2024-03-22T11:55:16Z

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

+  if (TLI.isOperationLegal(ISD::AVGCEILU, VT) ||
+      TLI.isOperationLegal(ISD::AVGCEILS, VT)) {


It might be best if this was two if's. AArch64 will have both ceil and floor, but some architectures will have only one and this could create nodes where they are not legal.

Ah, ok, actually my intent was simply avoid duplication of code, but it seems to be necessary.

RKSimon · 2024-03-22T12:20:14Z

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
  SDValue N0 = N->getOperand(0);
  EVT VT = N0.getValueType();
  SDLoc DL(N);
-  if (TLI.isOperationLegal(ISD::AVGCEILU, VT)) {
+  bool IsAVGCEILULegal = TLI.isOperationLegal(ISD::AVGCEILU, VT);
+  bool IsAVGCEILSLegal = TLI.isOperationLegal(ISD::AVGCEILS, VT);


Don't bother doing this - just keep the ifs separate instead of nesting them - you're not gaining anything, Pull out the SDValue A, B line and you're done.

davemgreen

Looks good to me. Thanks

RKSimon

One minor thing I've just noticed

RKSimon · 2024-03-24T09:07:47Z

llvm/test/CodeGen/AArch64/hadd-combine.ll

@@ -341,6 +341,18 @@ define <8 x i16> @sub_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
  ret <8 x i16> %res
 }

+define <8 x i16> @shsub_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {


shsub_fixedwidth_v4i32 -> srhadd_fixedwidth_v8i16 (its matching the shsub instruction for v8i16 types)

RKSimon · 2024-03-24T09:08:08Z

llvm/test/CodeGen/AArch64/hadd-combine.ll

@@ -879,6 +891,18 @@ define <8 x i16> @uhadd_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {
  ret <8 x i16> %res
 }

+define <8 x i16> @shadd_fixedwidth_v4i32(<8 x i16> %a0, <8 x i16> %a1)  {


shadd_fixedwidth_v4i32 -> shadd_fixedwidth_v8i16 (its matching the shsub instruction for v8i16 types)

RKSimon

LGTM - thank you!

github-actions · 2024-03-24T15:33:35Z

@houndlord Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns

af9f29d

llvmbot added backend:AArch64 llvm:SelectionDAG SelectionDAGISel as well labels Mar 22, 2024

RKSimon requested review from RKSimon, jayfoad and davemgreen March 22, 2024 10:14

RKSimon reviewed Mar 22, 2024

View reviewed changes

Merge AVGCEIL and AVGEFLOOR signed and unsigned variants.

37ebcd7

houndlord force-pushed the main branch from 131b5be to ad1e6fc Compare March 22, 2024 11:36

Fix bad scope.

0479afb

houndlord force-pushed the main branch from ad1e6fc to 0479afb Compare March 22, 2024 11:47

davemgreen reviewed Mar 22, 2024

View reviewed changes

Refactor AVGFLOOR and AVGCEIL 'if' conditions.

efd5a2d

RKSimon reviewed Mar 22, 2024

View reviewed changes

Revert back 'if' nesting.

54dc532

houndlord force-pushed the main branch from 84409e0 to 54dc532 Compare March 22, 2024 12:47

davemgreen approved these changes Mar 23, 2024

View reviewed changes

RKSimon requested changes Mar 24, 2024

View reviewed changes

Fix labels in tests.

1c1336f

RKSimon approved these changes Mar 24, 2024

View reviewed changes

RKSimon merged commit 9632e15 into llvm:main Mar 24, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns #86222

Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns #86222

houndlord commented Mar 22, 2024 •

edited by RKSimon

github-actions bot commented Mar 22, 2024

llvmbot commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited

RKSimon Mar 22, 2024

RKSimon Mar 22, 2024

davemgreen Mar 22, 2024

houndlord Mar 22, 2024

RKSimon Mar 22, 2024

davemgreen left a comment

RKSimon left a comment

RKSimon Mar 24, 2024

RKSimon Mar 24, 2024

RKSimon left a comment

github-actions bot commented Mar 24, 2024

		if (TLI.isOperationLegal(ISD::AVGCEILU, VT) \|\|
		TLI.isOperationLegal(ISD::AVGCEILS, VT)) {

Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns #86222

Match fixed width ISD::AVGFLOORS + ISD::AVGCEILS patterns #86222

Conversation

houndlord commented Mar 22, 2024 • edited by RKSimon

github-actions bot commented Mar 22, 2024

llvmbot commented Mar 22, 2024 • edited

github-actions bot commented Mar 22, 2024 • edited

RKSimon Mar 22, 2024

Choose a reason for hiding this comment

RKSimon Mar 22, 2024

Choose a reason for hiding this comment

davemgreen Mar 22, 2024

Choose a reason for hiding this comment

houndlord Mar 22, 2024

Choose a reason for hiding this comment

RKSimon Mar 22, 2024

Choose a reason for hiding this comment

davemgreen left a comment

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon Mar 24, 2024

Choose a reason for hiding this comment

RKSimon Mar 24, 2024

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 24, 2024

houndlord commented Mar 22, 2024 •

edited by RKSimon

llvmbot commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited