Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstCombine incorrectly rewrites indices of gep inbounds #44206

Closed
aqjune opened this issue Feb 10, 2020 · 5 comments
Closed

InstCombine incorrectly rewrites indices of gep inbounds #44206

aqjune opened this issue Feb 10, 2020 · 5 comments
Assignees
Labels

Comments

@aqjune
Copy link
Contributor

aqjune commented Feb 10, 2020

Bugzilla Link 44861
Version trunk
OS All
CC @LebedevRI,@preames,@RKSimon,@nikic,@nunoplopes,@rotateright

Extended Description

$ 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.

@aqjune
Copy link
Contributor Author

aqjune commented Feb 14, 2020

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.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@nunoplopes
Copy link
Member

The test case seems to have been fixed in LLVM 12.

@nikic
Copy link
Contributor

nikic commented May 26, 2022

This issue still exists with the right invocation: https://llvm.godbolt.org/z/j6zPoKa7E

@nikic nikic reopened this May 26, 2022
@nunoplopes
Copy link
Member

ouch, thank you @nikic!

@nikic nikic self-assigned this May 30, 2022
@nikic
Copy link
Contributor

nikic commented May 31, 2022

Candidate patch: https://reviews.llvm.org/D126687

@nikic nikic closed this as completed in 36cbdaa May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants