[PATCH] D116730: [AArch64][SVE] Remove Redundant aarch64.sve.convert.to.svbool

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

A couple of initial comments. It also looks like the code needs formatting.

I'm still thinking about the overall effect of the transform.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:388
+  return None;
+}
+
----------------
This function looks strange to me. What use does the first element of the pair have, in terms of how it varies. When would it return anything other than a dyncast of Val for example? I think you can effectively inline this function and in my view it will make `SolveRedundantSVBoolForBinOps` clearer by removing the indirection of `OptVal.first` and `OptVal.second`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:393
+static Optional<Instruction *> SolveRedundantSVBoolForBinOps(InstCombiner &IC,
+                                              IntrinsicInst &II) {
+  if (auto Opt = GetPredicatedBinOp(II.getOperand(0))){
----------------
For the comment, you could use an example to illustrate what is going on. It doesn't currently clearly explain the why or the what to me.

For the name, I don't think we have many things called 'solve', and I don't think of this operation as solving. I think it is a combine. 

As a suggestion, `tryCombineFromSVBoolBinOp` may be an improvement?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:395
+  if (auto Opt = GetPredicatedBinOp(II.getOperand(0))){
+    auto OptVal = Opt.getValue();
+    auto BinOp = OptVal.first;
----------------
A minor note on variable names: 'Opt' presumably stands for 'optional', but it's not a great name. What's lacking is that the name doesn't tell a reader what's in it. For example it's always Some(Value) in the body of the if, and never None, so it's not really optional, in fact it's the pair. But naming this Pair wouldn't be great either, since that wouldn't give any information to the reader about what's in it. If I find myself using such variable names it is usually a hint something can be improved. As suggested above, I think the optional pair can be removed entirely by inlining GetPredicatedBinOp.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:468
   return IC.replaceInstUsesWith(II, EarliestReplacement);
 }
 
----------------
The logic of this function could be clearer. `EarliestReplacement` is a detail of the first combine, when it succeeds. Combines which follow this should not refer to that detail, otherwise it is suggestive that these combines are related.

I think in this case the function should be structured like so:

```
instCombineConvertFromSVBool() {
  // ... try first combine ...
  // ... try second combine ...
  return None;
}
```

So that the code relating to an individual combine does not span other combines (as caused by the return statement here).

The main benefit of my suggestion is that further combines can be added without changing existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116730