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

InstCombine incorrectly folds 'gep(bitcast ptr), idx' into 'gep ptr, idx' #43666

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

InstCombine incorrectly folds 'gep(bitcast ptr), idx' into 'gep ptr, idx' #43666

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

Comments

@aqjune
Copy link
Contributor

aqjune commented Dec 17, 2019

Bugzilla Link 44321
Resolution FIXED
Resolved on Dec 21, 2019 07:37
Version trunk
OS All
CC @efriedma-quic,@nunoplopes,@rotateright
Fixed by commit(s) 79c7fa3

Extended Description

$ cat gep-vector.ll
define i32* @&#8203;bitcast_vec_to_array_gep(<7 x i32>* %x, i64 %y, i64 %z) {
  %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]*
  %gep = getelementptr [7 x i32], [7 x i32]* %arr_ptr, i64 %y, i64 %z
  ret i32* %gep
}
$ opt -instcombine gep-vector.ll -S -o -
define i32* @&#8203;bitcast_vec_to_array_gep(<7 x i32>* %x, i64 %y, i64 %z) {
  %gep = getelementptr <7 x i32>, <7 x i32>* %x, i64 %y, i64 %z
  ret i32* %gep
}

This is incorrect because DataLayout::getTypeAllocSize(< 7 x i32 >) and getTypeAllocSize([ 7 x i32 ]) may differ.

This can be double-checked by emitting assembly code before/after optimization.
https://godbolt.org/z/xB-p5u
Before optimization, rax = rdi + 28 * rsi + 4 * rdx
After instcombine, rax = rdi + 32 * rsi + 4 * rdx.
@aqjune
Copy link
Contributor Author

aqjune commented Dec 17, 2019

The t

@aqjune
Copy link
Contributor Author

aqjune commented Dec 17, 2019

Sorry for my mistake :(
The test was excerpted from test/Transforms/InstCombine/gep-vector.ll.

@efriedma-quic
Copy link
Collaborator

I agree this is a bug.

@rotateright
Copy link
Contributor

I introduced the bug here:
https://reviews.llvm.org/D44833

Using weird types in the tests helped expose it. In the common case where we have a power-of-2 vector/array and the alloc-sizes are the same, I think the transform should still be ok. I'll post a patch for review.

@rotateright
Copy link
Contributor

https://reviews.llvm.org/D71771

@rotateright
Copy link
Contributor

Should be fixed with:
tps://reviews.llvm.org/rG79c7fa31f3aa

@rotateright
Copy link
Contributor

Lost some chars on that link:
https://reviews.llvm.org/rG79c7fa31f3aa

@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
3 participants
@aqjune @rotateright @efriedma-quic