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

[Evandro Menezes via Phabricator via llvm-commits]

evandro added a subscriber: hfinkel.
evandro added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MacroFusion.cpp:158
+      return true;
+  }
+
----------------
fhahn wrote:
> 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.
> 
The change is needed because otherwise the default exit is premature, as other tests may match conditions that failed in one test.

I agree that this is growing unwieldy.  Perhaps the time to refactor this code in separate helper functions is nigh.  However, I also dislike how hard it is to add new fused pairs.  That's why I proposed D35228, which would simplify adding and maintaining fusion quite a bit IMO.  Hopefully, I'll be able to round it out as @hfinkel suggested in the coming weeks.


Repository:
  rL LLVM

https://reviews.llvm.org/D42392