[Thumb] Resolve FIXME: Transform "(and (shl x, c2), c1)" into "(shl (and x, c1>>c2), c2)"

[AtariDreams]

Transform "(and (shl x, c2), c1)" into "(shl (and x, c1>>c2), c2)" if "c1 >> c2" is a cheaper immediate than "c1" using HasLowerConstantMaterializationCost.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: AtariDreams (AtariDreams)

Changes

Transform "(and (shl x, c2) c1)" into "(shl (and x, c1>>c2), c2)" if "c1 >> c2" is a cheaper immediate than "c1" using HasLowerConstantMaterializationCost.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMISelLowering.cpp (+10-3)

diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index b98006ed0cb3f4..4d92add3738e29 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -14388,9 +14388,16 @@ static SDValue CombineANDShift(SDNode *N,
     }
   }
 
-  // FIXME: Transform "(and (shl x, c2) c1)" ->
-  // "(shl (and x, c1>>c2), c2)" if "c1 >> c2" is a cheaper immediate than
-  // c1.
+  // Transform "(and (shl x, c2) c1)" into "(shl (and x, c1>>c2), c2)"
+  // if "c1 >> c2" is a cheaper immediate than "c1"
+  if (HasLowerConstantMaterializationCost(C1 >> C2, C1, Subtarget)) {
+
+    SDValue And = DAG.getNode(ISD::AND, DL, MVT::i32, N0->getOperand(0),
+                              DAG.getConstant(C1 >> C2, DL, MVT::i32));
+    return DAG.getNode(ISD::SHL, DL, MVT::i32, And,
+                       DAG.getConstant(C2, DL, MVT::i32));
+  }
+
   return SDValue();
 }
 

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[AtariDreams]

@topperc Thoughts?

[AtariDreams]

@topperc Thoughts?

[TNorthover]

I think this one looks reasonable to me. Logic seems to check out and it looks for benefits before proceeding.

[AtariDreams]

@RKSimon what do you think about merging this?

[RKSimon]

@RKSimon what do you think about merging this?

I don't think its necessary - we're up top 18.1.6 now and this is a perf improvement, not a critical bugfix for a recent issue.

Better to focus on getting trunk ready for 19.X in a couple of months time.

[AtariDreams]

This wasn't proposed for 18.x @RKSimon.

[RKSimon]

Sorry, misunderstood your question