Quuxplusone
commented
8 years ago The failure mode looks similar to crbug.com/629141
Closed Quuxplusone closed 8 years ago
Quuxplusone
commented
8 years ago The failure mode looks similar to crbug.com/629141
Quuxplusone
commented
8 years ago Bisection points to r275561 - code hoisting pass based on GVN
To reproduce:
Build Release+Asserts Clang at this revision for 32-bit windows
Use that to configure a Release+Asserts 32-bit Windows build, and build llvm-
mc, then run it like:
D:/src/llvm/build.bootstrap/./bin\llvm-mc.EXE D:\src\llvm\test\MC\Sparc\sparc64-
ctrl-instructions.s -triple=sparc64-unknown-linux-gnu -show-encoding(In reply to comment #2)
> Bisection points to r275561 - code hoisting pass based on GVN
>
>
> To reproduce:
>
> Build Release+Asserts Clang at this revision for 32-bit windows
>
> Use that to configure a Release+Asserts 32-bit Windows build, and build
> llvm-mc, then run it like:
>
> D:/src/llvm/build.bootstrap/./bin\llvm-mc.EXE
> D:\src\llvm\test\MC\Sparc\sparc64-ctrl-instructions.s
> -triple=sparc64-unknown-linux-gnu -show-encoding
I do not have access to a windows 32-bit machine.
From this description it sounds like llvm-mc is miscompiled.
Would you be able to bisect the problem and report a reduced testcase?
Here is how I would do that:
Build a good llvm-mc with a good compiler and keep around all the .o files.
Build a bad llvm-mc with the faulty compiler.
Link together half of good and half of bad .o files and check whether it fails.
Iterate this step halving at each iteration the number of .o files that may
contain the miscompilation.
Once you have a .o file that is miscompiled, bisect on the number of GVN
expressions hoisted: compile that .cpp file with the same options and adding "-
mllvm -gvn-max-hoisted=10000 and then bisect again that number until finding N
for which llvm-mc passes and N+1 fails.
Finally get the output of "-mllvm -print-after-all -mllvm -print-before-all"
for both N and N+1.Thanks! I've bisected with -gvn-max-hoisted and found that the function that seems to be breaking is SparcAsmParser::parseMEMOperand
I'll attach the IR before and after hoisting of that function.
Quuxplusone
commented
8 years ago Attached before (16805 bytes, application/octet-stream): parseMEMOperand before hoisting
Quuxplusone
commented
8 years ago Attached after (17116 bytes, application/octet-stream): parseMEMOperand after hoisting
Quuxplusone
commented
8 years ago diffing before and after shows that it seems we're introducing a new alloca (%21) in the if.end18 block. This is problematic, because it gets inserted after an inalloca (%argmem) which is live there. I think that's the source of our bug, and it also explains why this only happens on 32-bit Windows.
Quuxplusone
commented
8 years ago I'm not exactly sure what's going on, but looking at the diff again, maybe it's
something like this:
in cond.true there is:
%20 = getelementptr inbounds <{ %"class.std::unique_ptr.203"*, i32, %"class.std::unique_ptr.203" }>, <{ %"class.std::unique_ptr.203"*, i32, %"class.std::unique_ptr.203" }>* %argmem, i32 0, i32 0, !dbg !27007^M
store %"class.std::unique_ptr.203"* %ref.tmp20, %"class.std::unique_ptr.203"** %20, align 4, !dbg !27007
%ref.tmp20 is an alloca in the entry block
When hoisting, the pass hoists both the gep and the store, cloning the value
(the alloca) and putting that at the hoist point. (GVNHoist.cpp's
makeOperandsAvailable)
Since that means inserting an alloca after a live inalloca, it breaks the
program, but it also seems bad in general to insert an alloca into the non-
entry block.
Maybe the pass should bail out when Val is an AllocInst? Or maybe it should
check if it's from the entry block and if so insert the clone there as well?Thanks Hans!
It is very helpful to see the diff.
I will have a closer look and I will submit a patch to fix the problem.(In reply to comment #9)
> Thanks Hans!
> It is very helpful to see the diff.
> I will have a closer look and I will submit a patch to fix the problem.
I sent out https://reviews.llvm.org/D22644Fixed in https://reviews.llvm.org/rL276358
before(16805 bytes, application/octet-stream)after(17116 bytes, application/octet-stream)