See below the invalid transformation of a bitcast to a gep inbounds. The input to the bitcast is a function argument, which could be out-of-bounds. llvm/test/Transforms/InstCombine/cast.ll define [4 x float]* @test27([9 x [4 x float]]* %A) { ; CHECK-NEXT: [[C:%.*]] = getelementptr inbounds [9 x [4 x float]], [9 x [4 x float]]* [[A:%.*]], i64 0, i64 0 %c = bitcast [9 x [4 x float]]* %A to [4 x float]* ret [4 x float]* %c }
No idea if this is the best fix, but it's the simplest: https://reviews.llvm.org/D68244
Looking at the changed tests, I think it looks ok. There's a little improvement that can be done in e.g. InstCombine/load-bitcast-vec.ll, which is to use the "dereferenceable(16)" tag on the argument to keep inbounds. Another easy-ish optimization is to keep inbounds if gep has only one use and the user is a memory access that is guaranteed to dereference the memory. Dereferencing a poison value or a OOB pointer doesn't really matter. I guess this could be a separate rewrite or something. We could think of other ways to recover inbounds tags if needed.
(In reply to Nuno Lopes from comment #2) > Looking at the changed tests, I think it looks ok. > There's a little improvement that can be done in e.g. > InstCombine/load-bitcast-vec.ll, which is to use the "dereferenceable(16)" > tag on the argument to keep inbounds. +1 > Another easy-ish optimization is to keep inbounds if gep has only one use > and the user is a memory access that is guaranteed to dereference the > memory. Attributor should deal with that.
(In reply to Roman Lebedev from comment #3) > (In reply to Nuno Lopes from comment #2) > > Looking at the changed tests, I think it looks ok. > > There's a little improvement that can be done in e.g. > > InstCombine/load-bitcast-vec.ll, which is to use the "dereferenceable(16)" > > tag on the argument to keep inbounds. > +1 Agreed. > > Another easy-ish optimization is to keep inbounds if gep has only one use > > and the user is a memory access that is guaranteed to dereference the > > memory. > Attributor should deal with that. Agreed, especially since you have to propagate dereferenceability backwards from a potential access. See https://reviews.llvm.org/D65402 :)
https://reviews.llvm.org/rL373847 There's an open question (for me at least) about the semantics of a null pointer in a non-default address space, so we probably need to answer that before closing this bug report.
(In reply to Sanjay Patel from comment #5) > https://reviews.llvm.org/rL373847 > > There's an open question (for me at least) about the semantics of a null > pointer in a non-default address space, so we probably need to answer that > before closing this bug report. That's a good point. There's a bit of confusion around that in multiple places in LLVM. My take on that is that we can't assume anything about null for AS != 0. That means that we can have a block allocated there or not. It's an address like any other. So while null in AS=0 is in bounds, null in AS != 0 isn't necessarily. Actually I don't even think we should have a null constant in AS != 0 because it just creates confusion; it's not a special value.
(In reply to Nuno Lopes from comment #6) > (In reply to Sanjay Patel from comment #5) > > https://reviews.llvm.org/rL373847 > > > > There's an open question (for me at least) about the semantics of a null > > pointer in a non-default address space, so we probably need to answer that > > before closing this bug report. > > That's a good point. There's a bit of confusion around that in multiple > places in LLVM. > My take on that is that we can't assume anything about null for AS != 0. > That means that we can have a block allocated there or not. It's an address > like any other. > > So while null in AS=0 is in bounds, null in AS != 0 isn't necessarily. > Actually I don't even think we should have a null constant in AS != 0 > because it just creates confusion; it's not a special value. I agree with that assessment, it matches my understanding.
https://reviews.llvm.org/rL374728