[PATCH] PR19958 wrong code at -O1 and above on x86_64-linux-gnu (InstCombine)
suyog sarda
sardask01 at gmail.com
Tue Jul 8 10:40:29 PDT 2014
Thanks Duncan for your quick reply :)
After looking at `SimplifyICmpInst`, the logic there seems disjoint
enough that I think it's better just to handle the cases redundantly
> here. Even though it's called directly from this function, it's hard to
> verify exactly how its transformations overlap with these ones. Unless
> you can show a non-negligible compile time improvement from switching to
> `assert`s, I think the clearer code wins.
>
> (It's a weak preference, so if anyone following along thinks `assert`
> is better here, pipe up!)
>
Seems better to include all the cases. I will update the patch accordingly.
>
> > Secondly, you're handling both `lshr` and `ashr`, which I think you can
> > do if you add the right logic -- but you're handling them identically.
> > This seems unlikely to be correct.
> >
> > Thirdly, it looks like you're matching both `exact` and non-`exact`
> > cases. This is possible -- with `exact`, the result is a poison value,
> > so you have some freedom -- but your comments only talk about `exact`,
> > and it's not clear that the code here is correct otherwise.
> >
> > Since, the special cases for exact were handled in 'instsimplify' for
> lshr and ashr,
> > i combined the common logic. The only difference was checking the
> equality after
> > calculating the logs, which i have handled as separate cases.
> >
> >
> > I have some more specific comments below.
> >
> > Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
> > ===================================================================
> > --- lib/Transforms/InstCombine/InstCombineCompares.cpp
> > +++ lib/Transforms/InstCombine/InstCombineCompares.cpp
> > @@ -2459,6 +2459,48 @@
> > return new ICmpInst(I.getPredicate(), A, B);
> > }
> >
> > + // PR19753:
> > + // (icmp eq/ne (ashr/lshr exact const2, A), const1) -> (icmp eq/ne
> A,
> > + // Log2(const2/const1)) -> (icmp eq/ne A, Log2(const2) -
> Log2(const1)).
> >
> > This would read better like this:
> >
> > // (icmp eq/ne (ashr/lshr exact const2, A), const1) ->
> > // (icmp eq/ne A, Log2(const2/const1)) ->
> > // (icmp eq/ne A, Log2(const2) - Log2(const1)).
> >
> > Will rectify this :)
> >
> >
> > + // TODO : Handle this for other icmp instructions.
> > + if (I.isEquality()) {
> > + ConstantInt *CI2;
> > + if (match(Op0, m_AShr(m_ConstantInt(CI2), m_Value(A))) ||
> > + match(Op0, m_LShr(m_ConstantInt(CI2), m_Value(A)))) {
> >
> > Shouldn't this be checking for `exact`?
> >
> > The only difference for 'exact' and 'non-exact' was checking the
> equality after
> > calculating the logs, which i have handled as separate cases below.
> >
> >
> > + APInt AP1 = CI->getValue();
> > + APInt AP2 = CI2->getValue();
> > + int Shift;
> > + // (icmp eq/ne (ashr/lshr exact const2, A), 0) ->
> > + // (icmp ugt A, Log2(const2)).
> > + // const2 = 0/ both const = 0 is already handled previously.
> >
> > Is this guaranteed, even if `-instcombine` is run on its own? Is that
> > simplification guaranteed to run before this one? If you're sure you
> > can guarantee this, you should use an `assert`:
> >
> > assert(AP2 && "Shift by 0 should be simplified by now");
> >
> > If not, you should handle it explicitly.
> >
> > As mentioned earlier, this is handled in 'Simplify'.
> > I will modify the code (assert or handle the case itself), as per you
> suggestion.
> >
> >
> > + if (!AP1) {
> > + Shift = AP2.logBase2();
> > + return new ICmpInst(I.ICMP_UGT, A,
> > + ConstantInt::get(A->getType(), Shift));
> > + }
> > + // if const1 = const2 -> icmp eq/ne A, 0
> > + if (CI == CI2)
> > + return new ICmpInst(I.getPredicate(), A,
> > + ConstantInt::getNullValue(A->getType()));
> > + // If both the constants are negative, take their positive to
> calculate
> > + // log.
> > + if (AP1.isNegative() || AP2.isNegative()) {
> > + AP1 = -AP1;
> > + AP2 = -AP2;
> > + }
> >
> > This logic reads strangely, and I think it's wrong for `lshr`.
> >
> > If you can guarantee that they are both negative or neither negative,
> > then this might be reasonable:
> >
> > if (isa<AShrOperator>(Op0) {
> > assert(AP1.isNegative() == AP2.isNegative() &&
> > "Different signs should be simplified by now");
> > if (AP1.isNegative()) {
> > AP1 = -AP1;
> > AP2 = -AP2;
> > }
> > }
> >
> > If you can't guarantee it, you should handle it explicitly instead.
> >
> > There should be '&&' instead of '||'. i will rectify it.
> >
> > For, both ashr/lshr "exact" if the constants have opposite sign,
> > 'Simplify' returns false, and we never reach this place.
>
> I'm not convinced about `lshr exact`. Consider:
>
> %Shifted = lshr exact i2 2, %A
> %Compare = icmp eq i2 %Shifted, 1
>
> Here, 2 is negative and 1 is positive, but the %Compare should be `i1 1`
> when %A is 1.
>
> If simplify gives `i1 0` for this, it sounds like a bug to me. Can you
> confirm?
>
Strangely, the output varies here.
cat test.ll
define i1 @main(i2 %a) {
%shr = lshr exact i2 2, %a
%cmp = icmp eq i2 %shr, 1
ret i1 %cmp
}
llvm/llvm/build/bin/opt -S -instcombine test.ll
; ModuleID = 'test.ll'
define i1 @main(i2 %a) {
%shr = lshr exact i2 -2, %a
%cmp = icmp eq i2 %shr, 1
ret i1 %cmp
}
llvm/llvm/build/bin/opt -S -instsimplify test.ll
; ModuleID = 'test.ll'
define i1 @main(i2 %a) {
%shr = lshr exact i2 -2, %a
%cmp = icmp eq i2 %shr, 1
ret i1 %cmp
}
As visible above, the 'Simplify' or 'Combine' doesn't return false.
However,
Consider following Test case :
cat test.ll
define i1 @main(i32 %a) {
%shr = lshr exact i32 -64, %a // One positive one negative
%cmp = icmp eq i32 %shr, 16
ret i1 %cmp
}
llvm/llvm/build/bin/opt -S -instsimplify test.ll
; ModuleID = 'test.ll'
define i1 @main(i32 %a) {
ret i1 false
}
llvm/llvm/build/bin/opt -S -instcombine test.ll
; ModuleID = 'test.ll'
define i1 @main(i32 %a) {
ret i1 false
}
It seems, 'Simplify' is behaving differently for 'extreme (MIN/MAX)' values
and values in 'middle'. Is this logically correct (doesn't seem so)? Or a
bug?
>
> > For ashr non-'exact', if the constants have opposite sign, 'Simplify'
> > returns false.
> >
> > For lshr non-'exact', if const2 is positive and const1 is negative,
> > 'Simplify' returns false.
>
> Adding the logic (or `assert`s) will make this more obvious :).
>
> > For lshr non-'exact', if const2 is negative and const1 is positive,
> > we will have to handle this case. I missed this, will handle it.
> >
>
--
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140708/44f34a48/attachment.html>
More information about the llvm-commits
mailing list