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

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added a comment.

In D121449#3379104 <https://reviews.llvm.org/D121449#3379104>, @paulwalker-arm wrote:

> An alternative suggested is to perhaps keep the DAGCombine but have it just canonicalise the shift pattern to use `ISD::AND` [1], that way I believe the existing code in `emitComparison` will do what you need.  If that works then that's my preferred option as it keeps things simple and relatively target agnostic.
>
> [1]
> `setcc (srl x, imm), 0, ne ==> setcc (and x, (-1 << imm)), 0, ne`

It looks when I do this in combine , and will be replaced to shift again soon.

Combining: t31: i32 = setcc t14, Constant:i64<0>, setne:ch
Is 0 legal add imm: yes
Creating constant: t34: i64 = Constant<-8192>
Creating new node: t35: i64 = and t2, Constant:i64<-8192>

Combining: t25: ch = setne

Combining: t20: i64 = Constant<0>

Combining: t14: i64 = srl t2, Constant:i64<13>



================
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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121449