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

Jump to bottom

Folding 'gep p, (q - p)' to q should check it is never used for loads & stores #43748

Closed
aqjune opened this issue Dec 29, 2019 · 7 comments
Closed

Folding 'gep p, (q - p)' to q should check it is never used for loads & stores #43748

aqjune opened this issue Dec 29, 2019 · 7 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@aqjune
Copy link
Contributor

aqjune commented Dec 29, 2019

Bugzilla Link 44403
Resolution FIXED
Resolved on Mar 15, 2021 08:33
Version trunk
OS All
CC @majnemer,@LebedevRI,@nunoplopes,@RalfJung,@nagisa,@rotateright

Extended Description

$ cat gep.ll 
define i8* @​f(i8* %p, i8* %q) {
  %i = ptrtoint i8* %p to i64
  %j = ptrtoint i8* %q to i64
  %diff = sub i64 %j, %i
  %p2 = getelementptr i8, i8* %p, i64 %diff
  ret i8* %p2
}
$ opt -instcombine gep.ll -S -o -
; ModuleID = 'gep.ll'
source_filename = "gep.ll"
define i8* @​f(i8* %p, i8* %q) {
  ret i8* %q
}

According to the GEP document:
https://llvm.org/docs/GetElementPtr.html#can-i-compute-the-distance-between-two-objects-and-add-that-value-to-one-address-to-compute-the-other-address
Replacing ‘gep p, (q - p)’ with q is invalid when it is used by memory access operations.
InstCombine and InstSimplify both do this.

llc gep.ll -o - emits an optimized assembly, so SelDag or MIR seem to already have optimizations for these this pattern if my understanding is correct?
@rotateright
Copy link
Contributor

I'm not familiar with the rules on this, so just adding a few notes and others can decide what to do:

  1. The cited LangRef section says: "ptrtoint and inttoptr provide an alternative way to do this which do not have this restriction". It would be helpful to show the exact alternative sequences, so it's clear what is and is not allowed.

  2. The transform in question is in InstSimplify and was added here:
    https://reviews.llvm.org/rL216586 (recommit)
    https://reviews.llvm.org/rL216439 (original commit)

@nunoplopes
Copy link
Member

I'm not familiar with the rules on this, so just adding a few notes and
others can decide what to do:

  1. The cited LangRef section says: "ptrtoint and inttoptr provide an
    alternative way to do this which do not have this restriction". It would be
    helpful to show the exact alternative sequences, so it's clear what is and
    is not allowed.

inttoptr disables a lot of optimizations in LLVM, because LLVM is quite conservative with this. So I would avoid those here.

  1. The transform in question is in InstSimplify and was added here:
    https://reviews.llvm.org/rL216586 (recommit)
    https://reviews.llvm.org/rL216439 (original commit)

Thanks for the pointers!
This transformation is not correct at the IR level. Only at later levels close to assembly, as in IR there's this notion of memory objects and pointers track from where they were derived from. Only when we get to assembly (not sure about SDAG), it becomes bytes only.

The solution seems to be to leave those GEPs/ptrtoint alone in the IR, and do this optimization at a later step. I don't know if SDAG already has an assembly-like memory model or if it's still higher-level like LLVM IR.

@nunoplopes
Copy link
Member

*** Bug llvm/llvm-bugzilla-archive#45444 has been marked as a duplicate of this bug. ***

@nunoplopes
Copy link
Member

Fixed by https://reviews.llvm.org/D98588

@nagisa
Copy link
Member

nagisa commented Mar 15, 2021

There's a patch for the same issue in InstSimplify at https://reviews.llvm.org/D98611 as well.

@nunoplopes
Copy link
Member

mentioned in issue llvm/llvm-bugzilla-archive#45444

@nikic
Copy link
Contributor

nikic commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#48577

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nikic @nagisa @nunoplopes @aqjune @rotateright