[PATCH] Transform OR of SELECTs to SELECT of ORs

Muhammad Tauqir Ahmad muhammad.t.ahmad at intel.com
Fri Feb 1 08:00:54 PST 2013


Forgot to reply-all.

If I don't disable this particular transform for vectors, it will
transform that into a vector select (C0 ? A : B) and that will end up
"generating awful code" for it.
If it was in fact fixed, the test would also have been fixed I assume (I hope).

- Muhammad Tauqir


On Fri, Feb 1, 2013 at 10:34 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Muhammad Tauqir Ahmad" <muhammad.t.ahmad at intel.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: reviews+D362+public+3c58d351ec48fa07 at llvm-reviews.chandlerc.com, llvm-commits at cs.uiuc.edu
>> Sent: Friday, February 1, 2013 9:21:51 AM
>> Subject: Re: [PATCH] Transform OR of SELECTs to SELECT of ORs
>>
>> In a comment on test32 in test/Transforms/InstCombine/or.ll (which
>> was
>> failing after I had implemented the transform), it says "Don't turn
>> this into a vector select until codegen matures to handle them
>> better"
>> which is why I am not turning this into a vector-select. That comment
>> was added by Chris Lattner and I took it to mean that any optmization
>> that converts this into a vector-select should not.
>
> It looks like that comment is now three years old:
> r95058 | lattner | 2010-02-01 20:43:51 -0600 (Mon, 01 Feb 2010) | 4 lines
>
> don't turn (A & (C0?-1:0)) | (B & ~(C0?-1:0)) ->  C0 ? A : B
> for vectors.  Codegen is generating awful code or segfaulting
> in various cases (e.g. PR6204).
>
> The specific bug report mentioned has been marked as resolved, but a duplicate of PR1784. PR1784 is still open, although it is not clear to me whether or not the problems with the in-memory representation of i1 vectors affects this transformation. I know that work on vector selects has been done over the past year at least.
>
>  -Hal
>
>>
>> - Muhammad Tauqir
>>
>>
>> On Fri, Feb 1, 2013 at 10:15 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>> > ----- Original Message -----
>> >> From: "Muhammad Tauqir Ahmad" <muhammad.t.ahmad at intel.com>
>> >> To: "Hal Finkel" <hfinkel at anl.gov>
>> >> Cc:
>> >> reviews+D362+public+3c58d351ec48fa07 at llvm-reviews.chandlerc.com,
>> >> llvm-commits at cs.uiuc.edu
>> >> Sent: Friday, February 1, 2013 9:08:36 AM
>> >> Subject: Re: [PATCH] Transform OR of SELECTs to SELECT of ORs
>> >>
>> >> I submitted a patch for
>> >> PR14664(http://llvm.org/bugs/show_bug.cgi?id=14664) which reversed
>> >> another optmization (more specifically, the tests in
>> >> logical-select.ll) so I am submitting this patch so that cases
>> >> like
>> >> the ones in logical-select.ll can be handled again through these
>> >> two
>> >> optmizations (the one in the patch for PR14664 + this patch).
>> >>
>> >> Does that answer your question as to why I am submitting this
>> >> patch?
>> >
>> > I don't think so. I'd specifically like to better understand the
>> > rationale for disabling the transformation for vector selects?
>> >
>> > Thanks again,
>> > Hal
>> >
>> >>
>> >> Thanks.
>> >> - Muhammad Tauqir
>> >>
>> >>
>> >> On Fri, Feb 1, 2013 at 9:59 AM, Hal Finkel <hfinkel at anl.gov>
>> >> wrote:
>> >> > ----- Original Message -----
>> >> >> From: "Muhammad Tauqir" <muhammadtauqir16 at gmail.com>
>> >> >> To: muhammadtauqir16 at gmail.com
>> >> >> Cc: llvm-commits at cs.uiuc.edu
>> >> >> Sent: Friday, February 1, 2013 7:59:28 AM
>> >> >> Subject: [PATCH] Transform OR of SELECTs to SELECT of ORs
>> >> >>
>> >> >> More descriptively, the transform is:
>> >> >> (or (bool?A:B),(bool?C:D)) --> (bool?(or A,C):(or B,D))
>> >> >>
>> >> >> By the time the OR is visited, both the SELECTs have been
>> >> >> visited
>> >> >> and
>> >> >> not optimized and the OR itself hasn't been transformed so we
>> >> >> do
>> >> >> this transform in the hopes that the new ORs will be optimized.
>> >> >>
>> >> >> The transform is explicitly disabled for vector-selects until
>> >> >> "codegen matures to handle them better".
>> >> >
>> >> > Can you be more specific about what needs to be improved, and on
>> >> > what targets?
>> >> >
>> >> > Thanks again,
>> >> > Hal
>> >> >
>> >> >>
>> >> >> http://llvm-reviews.chandlerc.com/D362
>> >> >>
>> >> >> Files:
>> >> >>   lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> >> >>   test/Transforms/InstCombine/logical-select.ll
>> >> >>
>> >> >> Index: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> >> >> ===================================================================
>> >> >> --- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> >> >> +++ lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> >> >> @@ -2071,6 +2071,22 @@
>> >> >>      return BinaryOperator::CreateOr(Inner, C1);
>> >> >>    }
>> >> >>
>> >> >> +  // Change (or (bool?A:B),(bool?C:D)) --> (bool?(or A,C):(or
>> >> >> B,D))
>> >> >> +  // Since this OR statement hasn't been optimized further
>> >> >> yet,
>> >> >> we
>> >> >> hope
>> >> >> +  // that this transformation will allow the new ORs to be
>> >> >> optimized.
>> >> >> +  {
>> >> >> +    Value *X = 0;
>> >> >> +    // FIXME: Disable vector-select until codegen matures to
>> >> >> handle
>> >> >> them better.
>> >> >> +    // See test32 in test/Transforms/InstCombine/or.ll
>> >> >> +    if (!I.getType()->isVectorTy())
>> >> >> +      if (match(Op0, m_Select(m_Value(X), m_Value(A),
>> >> >> m_Value(B)))
>> >> >> &&
>> >> >> +          match(Op1, m_Select(m_Value(X), m_Value(C),
>> >> >> m_Value(D))))
>> >> >> {
>> >> >> +        Value *orTrue = Builder->CreateOr(A, C);
>> >> >> +        Value *orFalse = Builder->CreateOr(B, D);
>> >> >> +        return SelectInst::Create(X, orTrue, orFalse);
>> >> >> +      }
>> >> >> +  }
>> >> >> +
>> >> >>    return Changed ? &I : 0;
>> >> >>  }
>> >> >>
>> >> >> Index: test/Transforms/InstCombine/logical-select.ll
>> >> >> ===================================================================
>> >> >> --- test/Transforms/InstCombine/logical-select.ll
>> >> >> +++ test/Transforms/InstCombine/logical-select.ll
>> >> >> @@ -10,10 +10,8 @@
>> >> >>    %j = or i32 %g, %i
>> >> >>    ret i32 %j
>> >> >>  ; CHECK: %e = icmp slt i32 %a, %b
>> >> >> -; CHECK-NEXT: %g = select i1 %e, i32 %c, i32 0
>> >> >> -; CHECK-NEXT: %i = select i1 %e, i32 0, i32 %d
>> >> >> -; CHECK-NEXT: %j = or i32 %g, %i
>> >> >> -; CHECK-NEXT: ret i32 %j
>> >> >> +; CHECK-NEXT: [[result:%.*]] = select i1 %e, i32 %c, i32 %d
>> >> >> +; CHECK-NEXT: ret i32 [[result]]
>> >> >>  }
>> >> >>  define i32 @bar(i32 %a, i32 %b, i32 %c, i32 %d) nounwind {
>> >> >>    %e = icmp slt i32 %a, %b
>> >> >> @@ -24,10 +22,8 @@
>> >> >>    %j = or i32 %i, %g
>> >> >>    ret i32 %j
>> >> >>  ; CHECK: %e = icmp slt i32 %a, %b
>> >> >> -; CHECK-NEXT: %g = select i1 %e, i32 %c, i32 0
>> >> >> -; CHECK-NEXT: %i = select i1 %e, i32 0, i32 %d
>> >> >> -; CHECK-NEXT: %j = or i32 %i, %g
>> >> >> -; CHECK-NEXT: ret i32 %j
>> >> >> +; CHECK-NEXT: [[result:%.*]] = select i1 %e, i32 %c, i32 %d
>> >> >> +; CHECK-NEXT: ret i32 [[result]]
>> >> >>  }
>> >> >>
>> >> >>  define i32 @goo(i32 %a, i32 %b, i32 %c, i32 %d) nounwind {
>> >> >> @@ -40,10 +36,8 @@
>> >> >>    %3 = or i32 %1, %2
>> >> >>    ret i32 %3
>> >> >>  ; CHECK: %0 = icmp slt i32 %a, %b
>> >> >> -; CHECK-NEXT: %1 = select i1 %0, i32 %c, i32 0
>> >> >> -; CHECK-NEXT: %2 = select i1 %0, i32 0, i32 %d
>> >> >> -; CHECK-NEXT: %3 = or i32 %1, %2
>> >> >> -; CHECK-NEXT: ret i32 %3
>> >> >> +; CHECK-NEXT: [[result:%.*]] = select i1 %0, i32 %c, i32 %d
>> >> >> +; CHECK-NEXT: ret i32 [[result]]
>> >> >>  }
>> >> >>  define i32 @poo(i32 %a, i32 %b, i32 %c, i32 %d) nounwind {
>> >> >>  entry:
>> >> >> @@ -55,10 +49,8 @@
>> >> >>    %3 = or i32 %1, %2
>> >> >>    ret i32 %3
>> >> >>  ; CHECK: %0 = icmp slt i32 %a, %b
>> >> >> -; CHECK-NEXT: %1 = select i1 %0, i32 %c, i32 0
>> >> >> -; CHECK-NEXT: %2 = select i1 %0, i32 0, i32 %d
>> >> >> -; CHECK-NEXT: %3 = or i32 %1, %2
>> >> >> -; CHECK-NEXT: ret i32 %3
>> >> >> +; CHECK-NEXT: [[result:%.*]] = select i1 %0, i32 %c, i32 %d
>> >> >> +; CHECK-NEXT: ret i32 [[result]]
>> >> >>  }
>> >> >>
>> >> >>  define i32 @par(i32 %a, i32 %b, i32 %c, i32 %d) nounwind {
>> >> >> @@ -71,8 +63,6 @@
>> >> >>    %3 = or i32 %1, %2
>> >> >>    ret i32 %3
>> >> >>  ; CHECK: %0 = icmp slt i32 %a, %b
>> >> >> -; CHECK-NEXT: %1 = select i1 %0, i32 %c, i32 0
>> >> >> -; CHECK-NEXT: %2 = select i1 %0, i32 0, i32 %d
>> >> >> -; CHECK-NEXT: %3 = or i32 %1, %2
>> >> >> -; CHECK-NEXT: ret i32 %3
>> >> >> +; CHECK-NEXT: [[result:%.*]] = select i1 %0, i32 %c, i32 %d
>> >> >> +; CHECK-NEXT: ret i32 [[result]]
>> >> >>  }
>> >> >>
>> >> >> _______________________________________________
>> >> >> llvm-commits mailing list
>> >> >> llvm-commits at cs.uiuc.edu
>> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >>
>> >>
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list