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

Matt Devereau via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 6 05:42:53 PST 2022


MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:388
+  return None;
+}
+
----------------
peterwaller-arm wrote:
> 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`.
My logic here was that I need to check that the initial convert.from.svbool operand is both an intrinsic and a binop intrinsic so I may as well return both pieces of work neatly in this helper function which might grow as we add more binops to the list. If you don't think this its worth it then inlining it is fine


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:393
+static Optional<Instruction *> SolveRedundantSVBoolForBinOps(InstCombiner &IC,
+                                              IntrinsicInst &II) {
+  if (auto Opt = GetPredicatedBinOp(II.getOperand(0))){
----------------
peterwaller-arm wrote:
> 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?
I'm not sure how I can give an example of what the code is addressing without pasting in the test i've added or adding a really descriptive comment that is harder to read than an example. I've changed it a bit to hopefully make it more clear.

Changing the function name sounds good.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:395
+  if (auto Opt = GetPredicatedBinOp(II.getOperand(0))){
+    auto OptVal = Opt.getValue();
+    auto BinOp = OptVal.first;
----------------
peterwaller-arm wrote:
> 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.
I've removed the pair now


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:468
   return IC.replaceInstUsesWith(II, EarliestReplacement);
 }
 
----------------
peterwaller-arm wrote:
> 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.
I've moved the tryCombineFromSVBoolBinOp call further up the function now


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

https://reviews.llvm.org/D116730



More information about the llvm-commits mailing list