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

[Sanjay Patel via Phabricator via cfe-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

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've pointed out a few minor potential improvements, otherwise LGTM.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:730
+
+  static constexpr auto NotFound = None;
+
----------------
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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:778
+
+  static constexpr auto FoundMismatch = nullptr;
+
----------------
Similar comment about the name as above for NotFound (and might be better to declare it on the next line after above?).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:780
+
+  enum class SourceAggegate {
+    /// When analyzing the value that was inserted into an aggregate, we did
----------------
Spelling - "SourceAggregate"


================
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:
----------------
Instead of switch, could we reduce to:
  if (Describe(SourceAggForElt) != SourceAggregate::Found)
    return SourceAggForElt;


================
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;
----------------
Best to give that "64" a name in case we want to experiment with other settings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85787