[PATCH] D48828: [InstSimplify] fold extracting from std::pair

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48828#1177277, @lebedev.ri wrote:

> Yeah, ok, i've convinced myself that this is ok. LGTM.
>  I do believe that the reasoning for this to be in instsimplify are sound (gvn needs it, running instcombine won't cut it.)
>
> @inouehrs please commit **tests** //now// (as of the current trunk, to not angry the bots).




1. Yes, I'd also like to see the tests get committed now with baseline CHECKs.
2. We've made a good argument for having this in instsimplify, but I don't think we answered a related question: does adding either or both of these to instsimplify allow us to remove code from instcombine? Hopefully, there are existing regression tests for instcombine to verify its transforms - would those tests pass when this patch is applied?
3. This is 2 independent patches in 1 review (1 transform for 'lshr'; 1 transform for 'and'). It's best if we split them into separate patches.
4. At least some of the simplify tests do not appear to be minimized. I would expect that all tests end with 'lshr' or 'and' since that is where the pattern matching starts.
5. Why does the code use isa<ConstantInt> rather than match(op, m_APInt(C))? Using the matcher would give us vector splat functionality for free IIUC.


Repository:
  rL LLVM

https://reviews.llvm.org/D48828