[PATCH] PR19958 wrong code at -O1 and above on x86_64-linux-gnu (InstCombine)
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Jul 8 09:53:09 PDT 2014
> On 2014-Jul-08, at 06:29, suyog sarda <sardask01 at gmail.com> wrote:
>
> 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?
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!)
> 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?
> 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.
>
> + 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
More information about the llvm-commits
mailing list