llc -combiner-alias-analysis reorders volatiles

Quuxplusone commented 13 years ago


Bugzilla Link	PR9851
Status	REOPENED
Importance	P normal
Reported by	Wolfgang Puffitsch (hausen@gmx.at)
Reported on	2011-05-05 13:43:20 -0700
Last modified on	2011-05-06 16:19:57 -0700
Version	2.9
Hardware	PC Linux
CC	baldrick@free.fr, efriedma@quicinc.com, llvm-bugs@lists.llvm.org, llvm@sunfishcode.online
Fixed by commit(s)
Attachments	`volatile-combine.ll` (179 bytes, application/octet-stream)
Blocks
Blocked by
See also

Created attachment 6546
Test case for reordering of volatiles.

llc reorders accesses to volatiles if -combiner-alias-analysis is enabled,
although volatiles should not be reordered with regard to each other.

Testcase, which miscompiles at least for sparc and ppc32:
define i32 @test() {
  %X = volatile load i32* inttoptr(i32 8 to i32*)
  %Y = volatile load i32* inttoptr(i32 12 to i32*)
  %Z = add i32 %Y, 1000
  ret i32 %Z
}

Correct code, from llc -march=ppc32
  lwz 3, 8(0)
  lwz 3, 12(0)
  addi 3, 3, 1000
  blr

Miscompiled code, from llc -march=ppc32 -combiner-alias-analysis
  lwz 3, 12(0)
  lwz 4, 8(0)
  addi 3, 3, 1000
  blr

Quuxplusone commented 13 years ago

Attached volatile-combine.ll (179 bytes, application/octet-stream): Test case for reordering of volatiles.

Quuxplusone commented 13 years ago

Note that -combiner-alias-analysis is an experimental option, and issues are expected.

Quuxplusone commented 13 years ago

As far as I know reordering volatiles is perfectly valid.  You need a memory
barrier if you want to enforce ordering.

Quuxplusone commented 13 years ago

(In reply to comment #2)
> As far as I know reordering volatiles is perfectly valid.  You need a memory
> barrier if you want to enforce ordering.

From LangRef: "The optimizers must not change the number of volatile operations
or change their order of execution relative to other volatile operations."

Quuxplusone commented 13 years ago

Eli is right, reordering these loads is invalid.

Quuxplusone / LLVMBugzillaTest

llc -combiner-alias-analysis reorders volatiles #10184