InstCombine incorrectly rewrites indices of gep inbounds


Bugzilla Link	PR44861
Status	NEW
Importance	P normal
Reported by	Juneyoung Lee (juneyoung.lee@sf.snu.ac.kr)
Reported on	2020-02-09 19:14:38 -0800
Last modified on	2020-02-14 06:05:47 -0800
Version	trunk
Hardware	PC All
CC	lebedev.ri@gmail.com, listmail@philipreames.com, llvm-bugs@lists.llvm.org, llvm-dev@redking.me.uk, nikita.ppv@gmail.com, nunoplopes@sapo.pt, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

$ cat input.ll
declare void @g(i8*)

define void @foo(i8* %ptr, i1 %cc1, i1 %cc2, i1 %cc3, i32 %arg) {
entry:
  br i1 %cc1, label %do.end, label %if.then

if.then:
  br i1 %cc2, label %do.body, label %do.end

do.body:
  %phi = phi i32 [ %arg, %if.then ]
  %phi.ext = zext i32 %phi to i64
  %ptr2 = getelementptr inbounds i8, i8* %ptr, i64 %phi.ext
  %ptr3 = getelementptr inbounds i8, i8* %ptr2, i64 -1
  call void @g(i8* %ptr3)
  br i1 %cc3, label %do.end, label %if.then

do.end:
  ret void
}

$ opt -instcombine -S -o - input.ll
declare void @g(i8*)

define void @foo(i8* %ptr, i1 %cc1, i1 %cc2, i1 %cc3, i32 %arg) {
  br i1 %cc1, label %do.end, label %if.then

if.then:                                          ; preds = %do.body, %entry
  br i1 %cc2, label %do.body, label %do.end

do.body:                                          ; preds = %if.then
  %phi.ext = zext i32 %arg to i64
  %ptr2 = getelementptr inbounds i8, i8* %ptr, i64 -1
  %ptr3 = getelementptr inbounds i8, i8* %ptr2, i64 %phi.ext
  call void @g(i8* nonnull %ptr3)
  br i1 %cc3, label %do.end, label %if.then

do.end:                                           ; preds = %do.body, %if.then, %entry
  ret void
}

If %arg is 1, ptr3 in the source is gep inb (gep inb ptr, 1), -1 whereas ptr3 in the target is gep inb (gep inb ptr, -1), 1. This is incorrect because ptr3 in the tgt may be poison whereas ptr3 in the source isn't.

I track down reason and it was because InstCombine::visitGetElementPtrInst try to swap index operands of geps when it can exhibit more opportunities to LICM:

/* at InstructionCombining.cpp */
...
    // Try to reassociate loop invariant GEP chains to enable LICM.
    if (LI && Src->getNumOperands() == 2 && GEP.getNumOperands() == 2 &&
        Src->hasOneUse()) {
...

ptr3's gep has a constant index (-1), so InstCombine chose to swap it with ptr2's index (%phi.ext) to enable code motion of the updated ptr2.

Here are a few solutions that I have:

(1) do this transformation when the two indices are known to have the same sign or either of those is zero. For example, if ptr3's index is 1 instead of -1, this transformation is allowed. (2) drop inbound flags when doing the transformation.

Quuxplusone / LLVMBugzillaTest

InstCombine incorrectly rewrites indices of gep inbounds #43831