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

Hiroshi Inoue via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jul 28 06:21:24 PDT 2018


inouehrs added a comment.

> Yes, I'd also like to see the tests get committed now with baseline CHECKs.

I have committed unit tests in https://reviews.llvm.org/rL338107

> 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?

I have investigated related instcombine patterns. But so far, I do not find something redundant with this instsimplify patch.
The pattern for 'and' is handled by `SimplifyDemandedInstructionBits` in instcombine and it has much wider coverage than this pattern.
The pattern for 'lshr' is handled by a combination of multiple patterns for `or` and shifts in instcombine. My instsimplify patch fully covers neither patterns in instcombine.

> 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.

I will commit the transformation in `lshr` and `and` separately.

> 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.

I fixed the test. I intend to make the generated code simpler.

> 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.

Fixed. Thank you for the suggestion.


https://reviews.llvm.org/D48828





More information about the llvm-commits mailing list