This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Remove Redundant aarch64.sve.convert.to.svbool
ClosedPublic

Authored by MattDevereau on Jan 6 2022, 2:21 AM.

Details

Summary

Generated code resulted in redundant aarch64.sve.convert.to.svbool
calls for AArch64 Binary Operations. Narrow the more precise operands
instead of widening the less precise operands

Diff Detail

Event Timeline

MattDevereau created this revision.Jan 6 2022, 2:21 AM
MattDevereau requested review of this revision.Jan 6 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 2:21 AM

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

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

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
427

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.

432

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?

434

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.

526

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.

MattDevereau marked 3 inline comments as done.

Removed GetPredicatedBinOp function and inlined it
Updated tryCombineFromSVBoolBinOp comment and function name
ran clang format

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
427

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

432

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.

434

I've removed the pair now

526

I've moved the tryCombineFromSVBoolBinOp call further up the function now

DavidTruby added inline comments.Jan 6 2022, 5:54 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
445

nit: you don't need the <> here it can just be IRBuilder

MattDevereau added inline comments.Jan 7 2022, 5:35 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
445

i'm compiling this locally with clang++11 with default flags, it errors when this is removed

Added tests for other vector types and updated function comment

Matt added a subscriber: Matt.Jan 7 2022, 7:29 AM
DavidTruby added inline comments.Jan 7 2022, 8:11 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
445

Interesting! clang 13 and gcc also fail to compile without them, but it works with C++17 instead of 14. I wonder why the empty brackets are necessary here... Anyway they do seem to be so ignore my comment :)

An offline discussion prompted a rewording of the function's comment

Add SVE intrinsics BIC, EOR, NAND, NOR, ORN, ORR to
tryCombineFromSVBoolBinOp, allowing them to remove
redundant convert.to.svbool calls

added other binops

peterwaller-arm accepted this revision.Jan 13 2022, 4:03 AM

LGTM with some editorial suggestions.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
419–434

Heads up, LLVM doesn't use c-style /* */ comments. https://llvm.org/docs/CodingStandards.html#comment-formatting

I've had a go at editing the text here with the goal of simplifying it.

454

You can elide all the break; here aside from the last one to shorten this switch a little.

This revision is now accepted and ready to land.Jan 13 2022, 4:03 AM
This revision was landed with ongoing or failed builds.Jan 17 2022, 6:36 AM
This revision was automatically updated to reflect the committed changes.