[PATCH] shl Optimization InstCombine

[Duncan P. N. Exon Smith]

> On 2014 Jul 16, at 22:29, sonam kumari <sonam.kumari at samsung.com> wrote:
> 
> Hi rafael, dexonsmith, nicholas,
> 
> This patch handles 
> (icmp eq/ne (shl const2, A), const1) -> (icmp eq/ne A, log2(const1) - log2(const2)).
> I wrote this patch for the equality comparison.
> For other comparison operators, it needs to handled.
> In the test cases I considered all the boundary conditions for the 32 bit integer and handled them.
> 
> Please help in reviewing this patch.
> Any comments/suggestions are most welcomed.
> 
> Thanks,
> Sonam.
> 
> http://reviews.llvm.org/D4553
> 
> Files:
>  lib/Transforms/InstCombine/InstCombineCompares.cpp
>  test/Transforms/InstCombine/icmp.ll
> <D4553.11556.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Hi Sonam,

Thanks for working on this.

Your patch needs a fair bit of work, so I'm just going to give you some
high-level comments for now.

 1. Most fundamentally, I'm not convinced that (even if coded correctly)
    this transformation has merit.  Can you explain the logic?

    Possibly it's valid at a high-level for `nuw` and `nsw` versions of
    `shl`, but I'd like you to explain why.

    E.g., I believe your would miscompile the following:

        `(icmp eq i4 (shl i4 0101, i4 %A), i4 0100)`

    This should be true if `%A` is `i4 2`.  What does your code give?
    Why?  Does this fit your model?

 2. The formatting is nowhere close to the coding standards [1].  Many
    of the style problems can be corrected automatically by Clang
    Format; I recommend looking into clang-format-diff.py [2] in
    particular.

    One that clang format can't fix is variable naming.  Please have a
    look at the relevant section [3] in the coding standards and fix up
    your variables.

 3. You have a fair bit of logic dealing with `isNegative()`, but that
    doesn't mean much for `shl` [4].  Perhaps the name of this method is
    misleading you: it's just telling you whether the MSB is set.

    In particular, your comments about "positive" and "negative" numbers
    look out of place.  The only time the concept of positive and
    negative make sense for `shl` is with the `nsw` flag when dealing
    with poison values [5].

 4. You deal directly with `2147483647` quite a bit -- this is the
    maximum unsigned value for `i31` and the maximum signed value for
    `i32`, but for everything else it's meaningless.  What are you
    really trying to say here?

    When you resubmit, include testcases for `i8` and `i64` that
    miscompile with the current patch.  (Why did they miscompile?)

 5. I haven't looked closely at your testcases, but I did notice one
    thing that's strange: they refer to a `%shr` variable.  Surely
    `%shl` would be less confusing?

[1]: http://llvm.org/docs/CodingStandards.html
[2]: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
[3]: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
[4]: http://llvm.org/docs/LangRef.html#shl-instruction
[5]: http://llvm.org/docs/LangRef.html#poison-values