[PATCH] D42392: [AArch64] Add new target feature to fuse conditional select

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: fhahn.
fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MacroFusion.cpp:158
+      return true;
+  }
+
----------------
Do I understand correctly and are all the changes from the top to here just to avoid returning `false` early? I think what the existing patterns do is: Either check the first or second opcode. If it's a match, check if the other opcode can be fused with it and return true/false. If the first check is fails, continue. After a quick look, I do not think any of the first checks overlap with the new checks you added in this patch (i.e. no other pattern checks if the SecondOpcode in [ CSELWr, CSELXr ]), so is it really necessary to change the logic above?

On a more general note, it seems like the list of patterns is growing quite large (especially together with D42393). I think going forward it would make it easier if we move the pattern checks in helper functions, where we can just have the switch's and early returns. So in `shouldScheduleAdjacent` we would just have something like

```
if (ST.hasFuseAES() && isAESPattern(Op1, Op2)) return true;
if (ST.hasArithmeticCbzFusion() && isCbzFusionPattern(Op1, Op2)) return true;
.....
if (ST.hasFuseCCSelect() && isCCSelectPattern) return true;
```

IMO this would be easier to read understand and maintain. That's just my personal opinion though, not sure how other people feel.

We could even get rid of all the ifs if we want and just have a single return with all the patterns matching functions we support.



Repository:
  rL LLVM

https://reviews.llvm.org/D42392