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

[Roman Lebedev via Phabricator via llvm-commits]

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