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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
