[PATCH] D117689: [AArch64][SVE] Fold vselect into predicated fmul, fsub and fadd

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:8468
+  ]>;
\ No newline at end of file

----------------
I don't think this should mimic the form of the other (SVE_...) names, since those are for defining patterns, whereas this is a `PatFrags`, which is a different kind of thing. So in my view the name should be distinct to try and avoid catching unsuspecting readers into another trap.

How about `EitherVSelectOrPassthruPatFrags`. I'm thinking along the lines of trying to avoid the error of `Or` reading as the binary operation by prefacing it with `Either`. 

Also, this doesn't look like the right place for this. I think a better location would be up near this, prehaps right at the end of that block before the `getSVEPseudoMap` definition. For good measure I might put a comment-block delimiter which separates it from the patterns above, and indicate in the comment that this "matches either an intrinsic, or a predicated operation with an all active predicate".

```
//===----------------------------------------------------------------------===//
// SVE pattern match helpers.
//===----------------------------------------------------------------------===//
```

Note also the missing trailing newline phabricator is complaining about.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117689