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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48828#1179343, @inouehrs wrote:

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


Commit - yes. But it wouldn't be bad to review (concurrently)  these things as two differentials, too.

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

I'm not sure of the exact problem (vector tests needed?), but added one nit.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:1289-1291
+  const APInt *ShAmt;
+  if (match(Op1, m_APInt(ShAmt)) &&
+      match(Op0, m_c_Or(m_NUWShl(m_Value(X), m_Specific(Op1)), m_Value(Y)))) {
----------------
I'm not sure this is better, or the full fix (tests needed.)
I would think you'd need
```
  const APInt *ShAmt0, *ShAmt1;
  if (match(Op1, m_APInt(ShAmt2)) &&
      match(Op0, m_c_Or(m_NUWShl(m_Value(X), m_APInt(ShAmt1)), m_Value(Y))) &&
      *ShAmt0 == *ShAmt1) {
    const APInt *ShAmt = ShAmt1;
```


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1846-1849
+      if (EffBitsY.isSubsetOf(*Mask) && !EffBitsX.intersects(*Mask))
+        return Y;
+      if (EffBitsX.isSubsetOf(*Mask) && !EffBitsY.intersects(*Mask))
+        return XShifted;
----------------
`intersects()` is commutative, so this shouldn't matter.


https://reviews.llvm.org/D48828