[llvm] r203862 - Fix a bug in InstCombine where we would incorrectly attempt to construct a

[Chandler Carruth]

Sorry fro the commit necromancy, but when removing the code that was
forming the bad bitcast here (and thus removing the utility of this test) I
got really confused by the underlying IR....

On Thu, Mar 13, 2014 at 3:51 PM, Owen Anderson <resistor at mac.com> wrote:

> Author: resistor
> Date: Thu Mar 13 17:51:43 2014
> New Revision: 203862
>
> URL: http://llvm.org/viewvc/llvm-project?rev=203862&view=rev
> Log:
> Fix a bug in InstCombine where we would incorrectly attempt to construct a
> bitcast between pointers of two different address spaces if they happened
> to have
> the same pointer size.
>

Ok, while generally this makes sense -- we can't blindly bitcast between
address spaces just because they have the same size -- the test case
doesn't look like valid IR to begin with and the code forming the "bad" bit
cast seems to be totally correct according to the spec of bitcasts. See
below:

Added: llvm/trunk/test/Transforms/InstCombine/load-addrspace-cast.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/load-addrspace-cast.ll?rev=203862&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/load-addrspace-cast.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/load-addrspace-cast.ll Thu Mar
> 13 17:51:43 2014
> @@ -0,0 +1,12 @@
> +; RUN: opt -instcombine -S < %s | FileCheck %s
> +target datalayout = "e-p:64:64:64-n8:16:32:64"
> +
> +define i32* @pointer_to_addrspace_pointer(i32 addrspace(1)** %x) nounwind
> {
> +; CHECK-LABEL: @pointer_to_addrspace_pointer(
> +; CHECK: load
> +; CHECK: addrspacecast
> +  %y = bitcast i32 addrspace(1)** %x to i32**
> +  %z = load i32** %y
>

So, why isn't this garbage in, garbage out? This code is blindly loading
the bits of an addrspace(1) pointer into an addrspace(0) pointer. That is
*exactly* the semantics provided by bitcast! Transforming this into a
bitcast seems completely "correct" in that it accurately represents the
operation in the input IR, and the problem seems to be that the input IR is
invalid.

Should code like this be rejected by the verifier? Or some other tool? Is
that possible? Could we at least document that rinsing a pointer through
memory from one address space to another is just as invalid as bitcasting?

My assumption is that this is just an overly reduced test case from a real
bug? But maybe not? I'm somewhat worried that instcombine may have been
papering over bugs somewhere else in LLVM by "fixing" a store -> load
"bitcast" of across address spaces into a proper addrspace cast.... Please
let me know if that is the case. I'm about to remove the code which was
doing this entirely, and if that exposes some deep underlying bug I can add
back in an addrspace_cast forming "fix" until the core issues is fixed.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141017/cd1567be/attachment.html>