lebedev.ri added a comment. @spatel thank you for taking a look!
In D85787#2216518 <https://reviews.llvm.org/D85787#2216518>, @spatel wrote: > In D85787#2214038 <https://reviews.llvm.org/D85787#2214038>, @lebedev.ri > wrote: > >> @ reviewers - i'm not so much interested in deep code/algo review, >> but more like in the general direction disscussion, like, is this okay for >> instcombine? :) > > The test diffs look great, and it seems to at least pay for itself in > compile-time impact, so I think it's a good direction. > There's always a question of "is this substantial enough (or would it grow > enough with the 'TODO' items) to be a stand-alone pass?". Yeah, i'm not sure. I don't really expect it to grow *that* much further after addressing TODO's. This is a weird mix of GVN and CSE, with PHI awareness ontop. It doesn't really fit anywhere. Also, i don't have any info on how making this fold a separate pass (and it's placement) would affect the results. > Haven't had a chance to look at the code itself yet. > Do we have tests where there are extra uses of the extracted values? Sure we do :) ; This fold does not care whether or not intermediate instructions have extra uses. define { i32, i32 } @test12({ i32, i32 } %srcagg) { in `llvm/test/Transforms/InstCombine/aggregate-reconstruction.ll`. 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
