instcombine incorrectly turns unsigned pointer comparision into signed comparison

[Quuxplusone]

Bugzilla Link	PR16483
Status	RESOLVED FIXED
Importance	P normal
Reported by	Richard Osborne (richard@xmos.com)
Reported on	2013-06-28 10:17:12 -0700
Last modified on	2013-07-08 03:27:23 -0700
Version	trunk
Hardware	PC Linux
CC	baldrick@free.fr, david.majnemer@gmail.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

If I give instcombine the following IR:

define i1 @f([1 x i8]* %a, [1 x i8]* %b) {
  %c = getelementptr [1 x i8]* %a, i32 0, i32 0
  %d = getelementptr [1 x i8]* %b, i32 0, i32 0
  %cmp = icmp ult i8* %c, %d
  ret i1 %cmp
}

opt -instcombine turns it into:

define i1 @f([1 x i8]* %a, [1 x i8]* %b) {
  %cmp = icmp slt [1 x i8]* %a, %b
  ret i1 %cmp
}

i.e it has turned an unsigned pointer comparision into a signed comparision
that will give different results with some inputs.

[Quuxplusone]

Line 650 in FoldGEPICmp [*]:

      // If all indices are the same, just compare the base pointers.
      if (IndicesTheSame)
        return new ICmpInst(ICmpInst::getSignedPredicate(Cond),
                            GEPLHS->getOperand(0), GEPRHS->getOperand(0));

We *always* use the signed version of the condition for some reason.

[*] http://llvm.org/viewvc/llvm-
project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?revision=185111&view=markup

[Quuxplusone]

While the code has danced around over the years, it looks like this was
introduced when LLVM switched from having setcc to icmp [*].

http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?r1=32690&r2=32751

[Quuxplusone]

A consequence of this is that the following two functions aren't the same!
struct blah {
  char thing[1];
};
bool f(struct blah *a, struct blah *b) {
  char *c = a->thing;
  char *d = b->thing;
  return c < d;
}

bool f2(struct blah *a, struct blah *b) {
  return a < b;
}

gives us:
define zeroext i1 @_Z1fP4blahS0_(%struct.blah* %a, %struct.blah* %b) #0 {
entry:
  %cmp = icmp slt %struct.blah* %a, %b
  ret i1 %cmp
}

; Function Attrs: nounwind readnone uwtable
define zeroext i1 @_Z2f2P4blahS0_(%struct.blah* %a, %struct.blah* %b) #0 {
entry:
  %cmp = icmp ult %struct.blah* %a, %b
  ret i1 %cmp
}

[Quuxplusone]

Fixed in r185259.