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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

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).
And please wait a bit (2 days?) before committing the transform itself (and the test //changes// themselves) in case @spatel / others want to comment.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:1843
+      const APInt EffBitsY = APInt::getLowBitsSet(Width, EffWidthY);
+      const APInt EffBitsX = APInt::getLowBitsSet(Width, EffWidthX) << ShiftCnt;
+      // If the mask is extracting all bits from X or Y as is, we can skip
----------------
Hmm, right, nice!
Even fits within 80-char width :)



================
Comment at: lib/Analysis/InstructionSimplify.cpp:1846-1849
+      if (EffBitsY.isSubsetOf(Mask) && !Mask.intersects(EffBitsX))
+        return Y;
+      if (EffBitsX.isSubsetOf(Mask) && !Mask.intersects(EffBitsY))
+        return XShifted;
----------------
> `isSubsetOf()` - This operation checks that all bits set in this APInt are also set in RHS.
So we check that the mask covers all the possibly-set bits of `X` / `Y`.
> `intersects()` - This operation tests if there are any pairs of corresponding bits between this APInt and RHS that are both set.
And we are checking that the mask does *not* cover any possibly-set bits of the `Y` / `X`.
Looks about right..



Repository:
  rL LLVM

https://reviews.llvm.org/D48828