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

[Duncan P. N. Exon Smith]

> On 2014 Jul 3, at 00:07, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> On 2014 Jul 2, at 14:33, Sanjay Patel <spatel at rotateright.com> wrote:
>> 
>> Sorry for the delay. LGTM...but considering that versions of this patch have caused miscompiles twice and I'm pretty new here, I think we should have someone with more instcombine experience give final approval in case I've missed anything.
>> 
>> http://reviews.llvm.org/D4068
>> 
> 
> Hi Suyog,
> 
> It seems like you might be able to go a little further without much
> work.

After thinking about it overnight, I think if you reverse the order of
the lg and divide, you'll avoid the expensive division.  As a bonus, it
looks to me like you can always combine the instructions if you do this
carefully.

I.e., here's some pseudo-code for `ashr`:

    // (icmp eq/ne (ashr exact C2, A), C1)
    APInt C1, C2;
    if (matchesAShrExact(..., C1, C2)) {
      if (!C1) {
        if (!C2)
          return "i1 1";
        return "i1 0";
      }
      if (!C2)
        return "i1 0";
      if (C1 == C2)
        return "icmp eq A, 0";
      if (C1.isNegative() != C2.isNegative())
        return "i1 0";
      if (C1.isNegative()) {
        C1 = -C1;
        C2 = -C2;
      }
      if (C1 > C2)
        return "i1 0";
      int LgC1 = C1.logBase2(),
          LgC2 = C2.logBase2();
      if (LgC1 == LgC2)
        return "i1 0";
      assert(LgC2 > LgC1);
      // This subtraction takes the place of your original division.
      int Shift = LgC2 - LgC1;
      if (C1.shl(Shift) == C2)
        return "icmp eq A, {Shift}";
      return "i1 0";
   }

I think other than the handling of `isNegative()`, the same logic works
for `lshr`.

I think you can use the same basic logic for non-`exact` right-shifts as
well.  I believe you just need to modify the initial 0-related checks
and reverse the direction of the shift, effectively masking out the
lower bits of C2 in the comparison at the end:

    // (icmp eq/ne (ashr C2, A), C1)
    APInt C1, C2;
    if (matchesAShrNotExact(..., C1, C2)) {
      if (!C1) {
        // Corner cases for C1 == 0 are different.
        if (!C2)
          return "i1 1";
        if (C2.isNegative())
          return "i1 0";
        int Shift = C2.logBase2();
        return "icmp gt A, {Shift};
      }
      if (!C2)
        return "i1 0";
      // ... same code as before, until:
      // Use lshr here, since we've canonicalized to +ve numbers.
      int Shift = LgC2 - LgC1;
      if (C1 == C2.lshr(Shift))
        return "icmp eq A, {Shift}";
      return "i1 0";
    }

Again, I think that aside from negative checking, the same basic logic
works for `lshr`.