[PATCH] shl Optimization InstCombine
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jul 21 19:30:08 PDT 2014
> 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.
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
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 . Many
of the style problems can be corrected automatically by Clang
Format; I recommend looking into clang-format-diff.py  in
One that clang format can't fix is variable naming. Please have a
look at the relevant section  in the coding standards and fix up
3. You have a fair bit of logic dealing with `isNegative()`, but that
doesn't mean much for `shl` . 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 .
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?
More information about the llvm-commits