search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Implicit control flow map broken assertion for readnone function #33910

Open Quuxplusone opened 7 years ago

Quuxplusone commented 7 years ago
Bugzilla Link PR34937
Status NEW
Importance P normal
Reported by Michael Ferguson (mpfergu@gmail.com)
Reported on 2017-10-12 18:03:00 -0700
Last modified on 2019-04-23 14:28:59 -0700
Version trunk
Hardware PC Linux
CC alexfh@google.com, dberlin@dberlin.org, hiraditya@msn.com, llvm-bugs@lists.llvm.org, max.kazantsev@azul.com
Fixed by commit(s)
Attachments pr34937.diff (4063 bytes, text/plain)
Blocks
Blocked by
See also

D37460 / r315440 seems to have caused a regression. Before this change, the following program compiled with opt -gvn minimal64.ll -S but after, it causes an assertion failure in the compiler. I've verified that this failure is still present on trunk as of r315579.

GVN.cpp:1073: bool llvm::GVN::PerformLoadPRE(llvm::LoadInst*, llvm::GVN::AvailValInBlkVect&, llvm::GVN::UnavailBlkVect&): Assertion `It->second->getParent() == TmpBB && "Implicit control flow map broken?"' failed.

Here is a carefully minimized test case (from a failure compiling with our frontend):

; ModuleID = 'simpler.ll' source_filename = "bugpoint-output-5fd3360.bc" target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu"

%ArrayImpl = type { i64, i64 addrspace(100), [1 x i64], [1 x i64], [1 x i64], i64, i64, double addrspace(100), double addrspace(100)*, i8, i64 }

; Function Attrs: readnone declare %ArrayImpl @getaddr_ArrayImpl(%ArrayImpl addrspace(100)) #0

; Function Attrs: readnone declare i64 @getaddr_i64(i64 addrspace(100)) #0

define hidden void @wrapon_fn173() { entry: %0 = call %ArrayImpl @getaddr_ArrayImpl(%ArrayImpl addrspace(100) undef) br label %loop

loop: %1 = call %ArrayImpl @getaddr_ArrayImpl(%ArrayImpl addrspace(100) undef) %2 = load i64 addrspace(100)*, i64 addrspace(100)* null, align 8 %3 = call i64 @getaddr_i64(i64 addrspace(100)* %2) br label %loop }

attributes #0 = { readnone }

To reproduce, put the above into minimal64.ll and run opt -gvn minimal64.ll -S.

Note that if the "readnone" attribute is removed from the getaddr functions, the test will pass.

I'd greatly appreciate any attention you can give to this matter.

Thanks!

Quuxplusone commented 7 years ago

Attached pr34937.diff (4063 bytes, text/plain): pr34937.diff

Quuxplusone commented 7 years ago 
It seems that the fix patch causes a lot of unit test failures with assertion

llvm/lib/Transforms/Scalar/GVN.cpp:576: uint32_t
llvm::GVN::ValueTable::lookup(llvm::Value*, bool) const: Assertion `VI !=
valueNumbering.end() && "Value not numbered?"' failed.

I tried to make a simple for for it but stumbled across other problems. How do
you think, does it make sense to revert  https://reviews.llvm.org/D37460 which
introduced the initial failure until the proper fix is made? I can do it, but
wanted to know your opinion because this patch fixes another potentially
dangerous GVN bug.
Quuxplusone commented 7 years ago

I've submitted a more conservative version of this fix for review as https://reviews.llvm.org/D38944 . It only fixes the contents of the map on instruction erasing and does not rework GVN marking/erasing mechanism (I think should be handled separately because it introduces its own troubles). I'd appreciate review comments.

Quuxplusone commented 7 years ago

The problematic change was reverted, I'm now shepherding the fix through review.

Quuxplusone commented 7 years ago

Test added as https://reviews.llvm.org/rL315976

Quuxplusone commented 5 years ago

Is the bug still relevant?