[PATCH] D121449: [AArch64] Combine ISD::SETCC into AArch64ISD::ANDS

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17307-17308
+    if (isNullConstant(RHS) && LHS->getOpcode() == ISD::SRL &&
+        LHS->hasOneUse() && isa<ConstantSDNode>(LHS->getOperand(1))) {
+      EVT TstVT = LHS->getOperand(0)->getValueType(0);
+      if (TstVT == MVT::i32 || TstVT == MVT::i64) {
----------------
bcl5980 wrote:
> paulwalker-arm wrote:
> > It's worth restricting this combine until later in the code generation pipeline because introducing target specific nodes (i.e. `AArch64ISD::`) early can result in loosing useful optimisations and/or sometimes introduces legalisation issues.  The above block get's away with it because it requires one of its inputs to be a a target specific node and so will naturally only trigger late.
> > 
> > I doubt it's necessary to restrict the combine until after legalisation, so it's likely just a case of adding
> > ```
> > if (DCI.isBeforeLegalize())
> >   return SDValue();
> > ```
> > at the top of this function so that you can be sure the types are legal and DAGCombine has had enough chance to do all the target independent combines.
> It looks ISD::SETCC will lower to AArch64ISD::CSEL in custom legalize. Do we need to add a new function like performCSELCombine to do combine on AArch64ISD::CSEL?
I'm sorry I find the function performCSELCombine. And another way is combine AArch64ISD::SUBS
SUBS (srl x, imm), 0 --> ANDS x, (-1 << imm)
which way do you think is better?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121449/new/

https://reviews.llvm.org/D121449