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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I've added some comments on the patch itself but these are likely redundant based on my top level question, which is whether the optimisation would be better placed in `emitComparison` which is used in several places including the lowering code for `ISD::SETCC`. It's responsible for deciding how best to set the flags and already includes emitting `AArch64ISD::ANDS` so your work feels like a natural extension.

As a side comment the diff itself lacks context and so, for example, things like the name of the changed function is unknown without searching a local checkout.  When manually uploading a patch to phabricator if you use `git show HEAD -U999999` the patch will include the context, which will be visible on phabricator. Alternatively you can use the `arcanist` tool to upload patches. See https://llvm.org/docs/Phabricator.html



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17286
+  if (Cond == ISD::SETNE) {
     SDLoc DL(N);
+    EVT VT = N->getValueType(0);
----------------
Personally I wouldn't bother with factoring out this condition because it makes the diff look bigger than it actually is and causes more indentation. I'd just add your new combine test in isolation. i.e.
```
if (Cond == ISD::SETNE && isOneConstant(RHS) &&
    LHS->getOpcode() == AArch64ISD::CSEL &&
    isNullConstant(LHS->getOperand(0)) && isOneConstant(LHS->getOperand(1)) &&
    LHS->hasOneUse()) {
```


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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17310-17311
+      if (TstVT == MVT::i32 || TstVT == MVT::i64) {
+        auto *SrlImmOp = dyn_cast<ConstantSDNode>(LHS->getOperand(1));
+        uint64_t TstImm = -1ULL << SrlImmOp->getZExtValue();
+        SDVTList VTs = DAG.getVTList(TstVT, MVT_CC);
----------------
Do you need to descend into the operands of `ISD::SRL`? Does `LHS->getValueType(0)` work? because the input and output types should match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121449