LLVM incorrectly hoisting loads

test 367_letnoescape fails under LLVM as a load of the !HpLim register is hoisted out of the loop. So yielding is never done.

What I am not sure about right now is the best way to fix. Loads in LLVM can be annotated in a few different ways to fix this and not sure which one is the most 'correct'.

All the following work:

• mark the load as volatile. (seems to give nicest code as well)
• mark the load as atomic with either monotonic or seq_cst ordering.
• mark the load as both volatile and atomic.

This bug while only affecting a single test case seems very serious and potentially indicative of a large problem. How well are we communicating the load/store threaded semantics to LLVM?

And what semantics do we need to communicate? I think we are fine other than the STG registers...

So making a bug for now as I don't know yet the best way to proceed without dedicating some time to reading LLVM docs and probably talking to the LLVM devs as the docs on the memory model are fairly confusing.

e.g., Code in question:

Bad version (LBB0_1 loops forever as load hoisted out):

r1Uf_info:                              # @r1Uf_info
# BB#0:                                 # %c1Vy
	movq	144(%r13), %rax
	decq	%r14
	.align	16, 0x90
.LBB0_1:                                # %tailrecurse
                                        # =>This Inner Loop Header: Depth=1
	incq	%r14
	testq	%rax, %rax
	jne	.LBB0_1
# BB#2:                                 # %c1VD
	movq	-8(%r13), %rax
	movl	$r1Uf_closure, %ebx
	jmpq	*%rax  # TAILCALL

Code when marked with atomic (either monatonic or seq_cst) or both atomic and volatile:

r1Uf_info:                              # @r1Uf_info
# BB#0:                                 # %c1Vy
	decq	%r14
	.align	16, 0x90
.LBB0_1:                                # %tailrecurse
                                        # =>This Inner Loop Header: Depth=1
	incq	%r14
	movq	144(%r13), %rax
	testq	%rax, %rax
	jne	.LBB0_1
# BB#2:                                 # %c1VD
	movq	-8(%r13), %rax
	movl	$r1Uf_closure, %ebx
	jmpq	*%rax  # TAILCALL

Code when marked volatile:

r1Uf_info:                              # @r1Uf_info
# BB#0:                                 # %c1Vy
	decq	%r14
	.align	16, 0x90
.LBB0_1:                                # %tailrecurse
                                        # =>This Inner Loop Header: Depth=1
	incq	%r14
	cmpq	$0, 144(%r13)
	jne	.LBB0_1
# BB#2:                                 # %c1VD
	movq	-8(%r13), %rax
	movl	$r1Uf_closure, %ebx
	jmpq	*%rax  # TAILCALL

changed weight to 5

added LLVM backend Tbug Trac import labels

changed the description

changed milestone to %7.8.1

Copying some info from an email (Memory model of Cmm?) with Simon Marlow so I can archive it finally:

The best reference for this is the code I wrote to identify conflicts in
the Cmm sinking pass, see the function 'conflicts':

http://www.haskell.org/ghc/dist/current/docs/html/libraries/ghc-7.7.20120902/src/CmmSink.html

But that won't tell you anything about whether reading a memory
location can be cached or not.

How does LLVM handle this?  I'm thinking that maybe we should have
an explicit "volatile load" operation, that would behave like a CmmLoad
but could not be hoisted.  It should be a CallishMachOp, because you
presumably want to specify ordering for a volatile load with respect to
other side-effecting operations.

http://llvm.org/docs/LangRef.html#i_load

LLVM docs on volatile:

Certain memory accesses, such as loads, stores, and llvm.memcpys may
be marked volatile. The optimizers must not change the number of
volatile operations or change their order of execution relative to
other volatile operations. The optimizers may change the order of
volatile operations relative to non-volatile operations. This is not
Java's "volatile" and has no cross-thread synchronization behavior.

mentioned in issue #7652 (closed)

changed milestone to %7.10.1

Moving to 7.10.1

mentioned in issue #9425 (closed)

mentioned in commit 11455684

removed milestone

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

changed milestone to %8.0.1

Milestone renamed

added incorrect runtime result label

added broken test label

added Pnormal label