[PATCH] D48828: [InstSimplify] fold extracting from std::pair (1/2)

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked an inline comment as done.
inouehrs added a comment.

> Please add/adjust the tests with baseline checks as a preliminary step; we don't want to lose those in case the code change gets reverted.

I have updated baseline checks.

> Seeing this diff on its own makes it clear that we're overspecifying the more general SimplifyDemandedBits transform (what about the case where the shifts are in the opposite order?). That should be noted as a code comment and in the commit message.*

I added comments. I will also mention in the commit message.

> It has been discussed before that SimplifyDemandedBits really shouldn't be included in InstCombine; it should be its own pass. If that structural change was made, would it make adjusting the optimization pipeline a more appealing solution than this?

For the original motivating examples on jump threading, it needs inter-BB optimization (e.g. code below). So If SimplifyDemandedBits pass can support inter-BB opt as well as intra-BB opt and executed before the jump threading, it will be more general than this patch.

  BB1:
    %shl = shl nuw i64 1, 32
    %or = or i64 %shl, %v
    br %BB2
  BB2:
    %phi = phi i64 [ %or, %BB1 ], ... 
    %shr = lshr i64 %phi, 32



================
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)))) {
----------------
lebedev.ri wrote:
> inouehrs wrote:
> > lebedev.ri wrote:
> > > 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;
> > > ```
> > Sorry but I cannot catch why you use `m_APInt` in the matcher and then compare the values instead of using `m_Specific`. What kind of code sequences you want to cover with this?
> I'm not sure it matters right now.
> `m_APInt()` could potentially (not right now) match splat constant with undef's - `<i32 42, i32 undef, i32 42>`
> But `m_Specific()` compares the pointers, not the underlying data. So if `ShAmt0` and `ShAmt1` are both splat,
> but have different `undef`s (e.g. only one of them has `undef` elements), they would not have the same constant.
> 
> Theoretically, that code in my comment would still match this case.
> But this does not matter right now since `m_APInt()` does not accept constants with undef elements.
I got it. I rewrite the code as you suggested for safety.


https://reviews.llvm.org/D48828