[PATCH] PR19958 wrong code at -O1 and above on x86_64-linux-gnu (InstCombine)

[suyog sarda]

Hi Duncan,

Thanks for your review :)



> Hi Suyog,
>
> Thanks for continuing to work on this.
>
> I saw your reply about certain simplifications being handled elsewhere,
> but I'm finding the assumptions in the code difficult to follow.


> Firstly, I think you should handle every case locally.  There are two
> ways to do this: with `assert`s for cases you can guarantee are handled
> elsewhere, and with control flow for everything else.  IMO, all the
> cases are so straightforward that there's no downside to just repeating
> the simplification work.
>

> As is, you're relying on transformations elsewhere to make this code
> correct.  It's nearly impossible to confirm that the transformation is
> logically sound, and what happens if someone removes (or disables) the
> other transformation(s)?
>

Actually, i have omitted the code for test cases which never reaches this
place.
All such omitted cases are handled in the 'SimplifyICmpInst' call which is
just at the start
of 'VisitICmp' function. I ran 'instsimplify' pass on such cases and
verified.

Since, for every 'Combine' function, we visit 'Simplify' first, i didn't
include the code for it.
If those 'Simplify' calls at the start are removed (in future may be),
then we will have to handle those cases in 'Combine'.

'Assert' seems a good option. But writing code for all the cases will
improve readability and flow of logic,
though may be redundant.

Can you please suggest, what should i select?


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

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.

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.


>
> +        Shift = AP2.logBase2() - AP1.logBase2();
> +        if ((cast<BinaryOperator>(Op0)->isExact()) && (AP1.shl(Shift) ==
> AP2))
> +          return new ICmpInst(I.getPredicate(), A,
> +                              ConstantInt::get(A->getType(), Shift));
> +        // Use lshr here, since we've canonicalized to +ve numbers.
> +        else if (AP1 == AP2.lshr(Shift))
>
> Remove the `else`.  This should simply be:
>
>     if (AP1 == AP2.lshr(Shift))
>
> +          return new ICmpInst(I.getPredicate(), A,
> +                              ConstantInt::get(A->getType(), Shift));
> +        else
>
> Remove the `else` and un-indent the following `return`.
>
> +          return ReplaceInstUsesWith(I,
> ConstantInt::getFalse(I.getType()));
> +      }
> +    }
> +
>      // If we have an icmp le or icmp ge instruction, turn it into the
>      // appropriate icmp lt or icmp gt instruction.  This allows us to
> rely on
>      // them being folded in the code below.  The SimplifyICmpInst code
> has
>

I will properly re-factor the code, once i get suggestion from you, whether
to use assert or to simply
include all the cases even if they are handled previously.

( Now, i am also inclined towards including all cases for simplicity in
logic flow.
I had not included earlier it just to avoid redundancy)

Awaiting for your suggestion :)


-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140708/aee1a4ce/attachment.html>