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