[PATCH] D85787: [InstCombine] Aggregate reconstruction simplification (PR47060)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@spatel thank you for taking a look!

In D85787#2220343 <https://reviews.llvm.org/D85787#2220343>, @spatel wrote:

> This seems similar in spirit to the recursive element tracking that we do in collectShuffleElements() in this file or foldIdentityShuffles() in InstSimplify, but a bit more complicated (because of the phi translation?).

I actually haven't seen those, but i would imagine that yes, having to deal with PHI's makes things slightly more complicated.

> I've pointed out a few minor potential improvements, otherwise LGTM.





================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:730
+
+  static constexpr auto NotFound = None;
+
----------------
spatel wrote:
> IIUC, you're intentionally giving the placeholder objects the same names as the enum values, but that confused me. Would it make sense to call this "InvalidValue" or similar?
> IIUC, you're intentionally giving the placeholder objects the same names as the enum values, but that confused me.

Yes. I first added `enum`, and then thought that it would be better to not
spell `None; // SourceAggegate::NotFound` / `nullptr; // SourceAggegate::FoundMismatch`
but literally just give them a name and spell them as close to the `enum`
values as possible, for obvious reasons of avoiding duplication/errors.

> Would it make sense to call this "InvalidValue" or similar?

Do we want to change both the `enum` values and the variable names,
or just variables to be different from the `enum` values?

`NotFound` seems like a good fit to me, because with it we literally mean
that we can't tell what scalar value that element of an aggregate has.

`InvalidValue` doesn't really have any connotation to me,
but it does look kinda sorta similar to `UndefValue`, which i want to avoid.

So i'm not sure here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:855
+      // Okay, what have we found? Does that correlate with previous findings?
+      switch (Describe(SourceAggregateForElement)) {
+      case SourceAggegate::NotFound:
----------------
spatel wrote:
> Instead of switch, could we reduce to:
>   if (Describe(SourceAggForElt) != SourceAggregate::Found)
>     return SourceAggForElt;
Hm, i guess, thanks!
But we still need the inner `switch`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:907
+  // Don't bother if there are more than 64 predecessors.
+  if (UseBB->hasNPredecessorsOrMore(64 + 1))
+    return nullptr;
----------------
spatel wrote:
> Best to give that "64" a name in case we want to experiment with other settings.
Do you mean a `cl::opt`, or just a local `static const` variable?
I'll do the latter for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85787