Update LLVM to pull in patch that removes extraneous null check. by luqmana · Pull Request #40914 · rust-lang/rust

luqmana · 2017-03-29T22:29:50Z

rust-highfive · 2017-03-29T22:30:02Z

(rust_highfive has picked a reviewer for you, use r? to override)

arielb1 · 2017-03-30T11:02:45Z

Don't you want just -O?

True, left over from copy/paste of copyright preamble and compile-flags. Interestingly most (if not all?) the codegen tests seem to use no-prepopulate-passes.

That's because in most cases we are interested in checking our codegen, not LLVM's optimizations.

alexcrichton · 2017-03-30T16:42:51Z

Well it looks like the test works! (this is failing on the llvm 3.7 builder)

I think this may just need a // tag at the top (// min-llvm-version 3.7 or something like that I think).

Other than that r=me

arielb1 · 2017-03-30T18:44:49Z

@alexcrichton

We want to ignore the test on all non-local LLVMs - after all, no public LLVM released yet includes the optimization.

luqmana · 2017-03-30T20:01:28Z

@alexcrichton @arielb1 Hmm should we just use a version 4.1 and live with it not running for a while or do what a couple other tests seem to do and just specify 3.8? I'm not sure how many are actually in the llvm 3.8 branch.

alexcrichton · 2017-03-31T05:52:28Z

I don't particularly care how the test is written. If it gets past the bots and it runs on at least one bot that's all I really care about.

arielb1 · 2017-03-31T12:00:16Z

@luqmana

Distributions use their own LLVM, which does not include our patch.

alexcrichton · 2017-04-01T21:45:06Z

I picked up this LLVM update in the beta backport (just did a reset to the current master) and it looks like this may fail during the bootstrap?

https://travis-ci.org/rust-lang/rust/jobs/217577161

I'm not 100% sure it's caused by this, but it seems suspicious

luqmana · 2017-04-03T18:05:54Z

@alexcrichton hmm, i'll give a look into that.

luqmana · 2017-04-06T00:04:13Z

@alexcrichton I tried building beta and wasn't able to reproduce that failure?

TimNN · 2017-04-06T05:56:29Z

@luqmana: Did you build with LLVM assertions enabled?

Edit: Also, the failure was while building x86_64-gnu-incremental so maybe this occurs only if incremental compilation is enabled?

luqmana · 2017-04-07T05:14:39Z

@TimNN Aha! I didn't have assertions on. I must've disabled them at some point.

Enabling those I was able to reproduce the error @alexcrichton had. I minimized the IR with bugpoint:

; ModuleID = 'bugpoint-reduced-simplified.bc'
source_filename = "bugpoint-output-7eb0c3b.bc"targettriple = "x86_64-unknown-linux-gnu"%foo = type { %bar*, %baz* }
%bar = type { %bil*, i8*, %baz* }
%bil = type { i32, [0 x i32], [1 x i32] }
%baz = type { i8*, i64 }
definefastccvoid@fail_sroa() {
entry-block:
%arg5 = alloca%foo, align8%0 = bitcast%foo*%arg5toi64*storei64undef, i64*%0, align8%1 = getelementptrinbounds%foo, %foo*%arg5, i640, i320%2 = load%bar*, %bar**%1, align8, !nonnull!0unreachable
}
!0 = !{}

Running it with opt -sroa I get (from LLVM in tree):

opt: /export/scratch/laden/rust/src/llvm/lib/Analysis/ValueTracking.cpp:3202: bool llvm::isKnownNonNull(const llvm::Value*): Assertion `V->getType()->isPointerTy() && "V must be pointer type"' failed.
#0 0x0000562780420ee5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x1469ee5)
#1 0x000056278041eb6e llvm::sys::RunSignalHandlers() (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x1467b6e)
#2 0x000056278041ecbc SignalHandler(int) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x1467cbc)
#3 0x00007f5b576da0c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x110c0)
#4 0x00007f5b566b4fdf gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x32fdf)
#5 0x00007f5b566b640a abort (/lib/x86_64-linux-gnu/libc.so.6+0x3440a)
#6 0x00007f5b566ade47 (/lib/x86_64-linux-gnu/libc.so.6+0x2be47)
#7 0x00007f5b566adef2 (/lib/x86_64-linux-gnu/libc.so.6+0x2bef2)
#8 0x000056277fc476b0 llvm::isKnownNonNull(llvm::Value const*) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0xc906b0)
#9 0x000056277fc47782 llvm::isKnownNonNullAt(llvm::Value const*, llvm::Instruction const*, llvm::DominatorTree const*) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0xc90782)
#10 0x00005627804bf4d0 (anonymous namespace)::PromoteMem2Reg::run() (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x15084d0)
#11 0x00005627804c094b llvm::PromoteMemToReg(llvm::ArrayRef<llvm::AllocaInst*>, llvm::DominatorTree&, llvm::AliasSetTracker*, llvm::AssumptionCache*) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x150994b)
#12 0x000056278033e1fb llvm::SROA::promoteAllocas(llvm::Function&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x13871fb)
#13 0x0000562780355e6d llvm::SROA::runImpl(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x139ee6d)
#14 0x00005627803570dd llvm::sroa::SROALegacyPass::runOnFunction(llvm::Function&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x13a00dd)
#15 0x000056277fffae33 llvm::FPPassManager::runOnFunction(llvm::Function&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x1043e33)
#16 0x000056277fffb1eb llvm::FPPassManager::runOnModule(llvm::Module&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x10441eb)
#17 0x000056277fffb521 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x1044521)
#18 0x000056277f3e5492 main (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x42e492)
#19 0x00007f5b566a22b1 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b1)
#20 0x000056277f43670a _start (/scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt+0x47f70a)
Stack dump:
0. Program arguments: /scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt bad.ll -S -sroa
1. Running pass 'Function Pass Manager' on module 'bad.ll'.
2. Running pass 'SROA' on function '@fail_sroa'
[1] 21755 abort /scratch/laden/rust/build/x86_64-unknown-linux-gnu/llvm/bin/opt bad.ll -S

But, with LLVM from head the assertion is not triggered.

TimNN · 2017-04-07T06:38:34Z

@luqmana: If you want I can pick up the patch in the LLVM 4.0 update (#40123), at least to check if the assertions happens there.

luqmana · 2017-04-07T06:40:44Z

@TimNN That'd be great, thanks!

TimNN · 2017-04-07T16:48:00Z

I tested this with LLVM 4.0:

The minimised test case no longer fails
I'm still getting rustc: /checkout/src/llvm/lib/Analysis/ValueTracking.cpp:3368: bool llvm::isKnownNonNull(const llvm::Value*): Assertion `V->getType()->isPointerTy() && "V must be pointer type"' failed. when compiling core

shepmaster · 2017-04-14T14:28:19Z

@luqmana @TimNN what do we see as the next steps for this PR?

arielb1 · 2017-04-15T15:39:07Z

@shepmaster

Someone has to figure out why this causes LLVM assertions and fix that.

luqmana · 2017-04-15T22:11:19Z

@shepmaster @arielb1 I'll hopefully get to it when I'm done in the next week or two.

@TimNN Thanks for testing. Mind pushing a branch somewhere so I can reproduce?

TimNN · 2017-04-19T14:29:00Z

For the record, a short transcription for irc:


6:39 pm Luqman
TimNN: which was the branch you tested gh40914 with? I'd like to see figure out the cause of the assertion
6:39 pm [o__o]
Update LLVM to pull in patch that removes extraneous null check.: https://github.com/rust-lang/rust/pull/40914
6:40 pm TimNN
llvm40 in my rust-lang fork
6:40 pm
although I just backported a few llvm patches, which haven't been tested with
6:41 pm
so it would probably be best if you used llvm40 without the most recent commit

bors · 2017-04-25T01:25:42Z

☔ The latest upstream changes (presumably #40123) made this pull request unmergeable. Please resolve the merge conflicts.

arielb1 · 2017-04-25T09:44:48Z

@luqmana

LLVM 4.0 has just landed. Can you make it work with that?

arielb1 · 2017-05-02T09:16:08Z

@luqmana are you going to be doing this? Should I fix this PR myself?

luqmana · 2017-05-02T17:58:24Z

@arielb1 I was planning to get to it soon but you're also welcome to proceed. I did have a bit of time the other day and rebased on the llvm4.0 branch, but ended up finding that the extra null check reappeared. I didn't have time to investigate unfortunately.

arielb1 · 2017-05-09T08:50:19Z

I'll take up this PR myself.

Mark-Simulacrum · 2017-05-14T15:42:34Z

@arielb1 Any update on taking this PR up yourself?

arielb1 · 2017-05-16T18:24:16Z

status: waiting until I get on top of regressions etc.

Mark-Simulacrum · 2017-05-18T02:08:44Z

Note: When redoing the backport, if someone could check if #38349 is fixed as well that would be great.

arielb1 · 2017-05-23T22:14:47Z

status: currently dealing with a few more important backlog items. I'll get to this.

aidanhs · 2017-06-01T00:45:33Z

Hi @arielb1, just want to make sure this hasn't dropped off the bottom of your backlog :)

arielb1 · 2017-06-06T11:41:20Z

@aidanhs

Actually I'm doing some LLVM stuff right now, so it might skip upwards. I'm just busy with some personal stuff.

hanna-kruppe · 2017-06-06T17:23:27Z

FWIW the upstream version of this patch seems to cause an assertion failure in LLVM for me when compiling libcore [1]. Someone else (@vadimcn?) has already reported this upstream earlier and minified it to SROA incorrectly putting !nonnull on an integer load: https://bugs.llvm.org/show_bug.cgi?id=32902. While I haven't rigorously experimented and don't know that code at all, the the SROA changes look suspicious and indeed removing those two lines fixes the assertion failure for me.

PS: I'd report these findings on LLVM bug, but I don't have an account. Would be great if someone could pass it on.

[1] To clarify, this is with an LLVM fork that tracks upstream much more closely than we do.

Edit: Ha, I only just noticed that this assertion failure has been seen here too 😅 I somehow missed that and thought the problem was something different.

arielb1 · 2017-06-13T20:36:32Z

Let's see how that goes.

arielb1 · 2017-06-14T21:55:10Z

This patch does not actually help the original case - now it's EarlyCSE that is eating our metadata. It turns

define internal i8* @"_ZN91_$LT$core..slice..Iter$LT$$u27$a$C$$u20$T$GT$$u20$as$u20$core..iter..iterator..Iterator$GT$4next
17hd95df97449b11211E"(%"core::slice::Iter<f32>"* dereferenceable(16)) unnamed_addr #0 {
entry-block:
%1 = call i64 @_ZN4core3mem7size_of17h0356fd359200dd41E()
%2 = icmp ne i64 %1, 0
br i1 %2, label %bb2, label %bb8
bb2: ; preds = %entry-block
%3 = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i32 0, i32 0
%4 = load float*, float** %3
%5 = call zeroext i1 @"_ZN4core3ptr33_$LT$impl$u20$$BP$const$u20$T$GT$7is_null17h852e1f4ae132da8fE"(float* %4)
%6 = xor i1 %5, true
call void @llvm.assume(i1 %6)
%7 = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i32 0, i32 1
%8 = load float*, float** %7
%9 = call zeroext i1 @"_ZN4core3ptr33_$LT$impl$u20$$BP$const$u20$T$GT$7is_null17h852e1f4ae132da8fE"(float* %8)
%10 = xor i1 %9, true
call void @llvm.assume(i1 %10)
br label %bb8
...

Into

define internal i8* @"_ZN91_$LT$core..slice..Iter$LT$$u27$a$C$$u20$T$GT$$u20$as$u20$core..iter..iterator..Iterator$GT$4next17hd95df97449b11211E"(%"core::slice::Iter<f32>"* dereferenceable(16)) unnamed_addr #0 {
entry-block:
%1 = call i64 @_ZN4core3mem7size_of17h0356fd359200dd41E()
%2 = icmp ne i64 %1, 0
br i1 %2, label %bb2, label %bb8
bb2: ; preds = %entry-block
%3 = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i32 0, i32 0
%4 = load float*, float** %3
%5 = call zeroext i1 @"_ZN4core3ptr33_$LT$impl$u20$$BP$const$u20$T$GT$7is_null17h852e1f4ae132da8fE"(float* %4)
%6 = getelementptr inbounds %"core::slice::Iter<f32>", %"core::slice::Iter<f32>"* %0, i32 0, i32 1
%7 = load float*, float** %6
%8 = call zeroext i1 @"_ZN4core3ptr33_$LT$impl$u20$$BP$const$u20$T$GT$7is_null17h852e1f4ae132da8fE"(float* %7)
br label %bb8

That's... odd. Why is it eating our assumptions?

arielb1 · 2017-06-14T22:00:05Z

Minified:

; RUN: opt -S -early-cse < %s | FileCheck %sdeclarei1@food()
declarevoid@llvm.assume(i1)
; CHECK-LABEL: define void @hungry()definevoid@hungry() {
entry-block:
%0 = calli1@food()
%1 = xori1%0, true; CHECK: call void @llvm.assumecallvoid@llvm.assume(i1%1)
retvoid
}

Passes on 3.9, fails on 4.0.

arielb1 · 2017-06-16T18:24:16Z

The LLVM assertion in https://bugs.llvm.org/show_bug.cgi?id=32902 is fixed by my https://reviews.llvm.org/D34285, which is awaiting review by LLVM.

arielb1 · 2017-06-20T19:59:40Z

status: still waiting for LLVM review by @chandlerc.

arielb1 · 2017-06-27T20:33:53Z

@chandlerc had committed llvm-mirror/llvm@7df0651, so this is fixed on LLVM trunk! I'll backport this to our LLVM tomorrow.

Mark-Simulacrum · 2017-07-02T00:49:54Z

This is waiting on rust-lang/llvm#90.

aidanhs · 2017-07-05T18:05:31Z

rust-lang/llvm#90 has landed.

arielb1 · 2017-07-11T10:51:01Z

superseded by #43026.

rust-highfive assigned alexcrichton Mar 29, 2017

arielb1 reviewed Mar 30, 2017
View reviewed changes

Update LLVM to pull in patch that removes extraneous null check.
3fb191c

luqmana force-pushed the 37945-update-llvm branch from 5c1ca0a to 3fb191cCompare March 30, 2017 13:08

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 15, 2017

Mark-Simulacrum mentioned this pull request May 18, 2017
Identity Result match mapping should optimize to an identity function #38349
Closed

arielb1 mentioned this pull request Jun 15, 2017
Extra null check in slice iterator's .next().is_none()#37945
Closed

arielb1 closed this Jul 11, 2017

Uh oh!

Uh oh!

Conversation

luqmana commented Mar 29, 2017

Uh oh!

rust-highfive commented Mar 29, 2017

Uh oh!

arielb1Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

luqmanaMar 30, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 30, 2017

Uh oh!

arielb1 commented Mar 30, 2017

Uh oh!

luqmana commented Mar 30, 2017

Uh oh!

alexcrichton commented Mar 31, 2017

Uh oh!

arielb1 commented Mar 31, 2017

Uh oh!

alexcrichton commented Apr 1, 2017

Uh oh!

luqmana commented Apr 3, 2017

Uh oh!

luqmana commented Apr 6, 2017

Uh oh!

TimNN commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luqmana commented Apr 7, 2017

Uh oh!

TimNN commented Apr 7, 2017

Uh oh!

luqmana commented Apr 7, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimNN commented Apr 7, 2017

Uh oh!

shepmaster commented Apr 14, 2017

Uh oh!

arielb1 commented Apr 15, 2017

Uh oh!

luqmana commented Apr 15, 2017

Uh oh!

TimNN commented Apr 19, 2017

Uh oh!

bors commented Apr 25, 2017

Uh oh!

arielb1 commented Apr 25, 2017

Uh oh!

arielb1 commented May 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luqmana commented May 2, 2017

Uh oh!

arielb1 commented May 9, 2017

Uh oh!

Mark-Simulacrum commented May 14, 2017

Uh oh!

arielb1 commented May 16, 2017

Uh oh!

Mark-Simulacrum commented May 18, 2017

Uh oh!

arielb1 commented May 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aidanhs commented Jun 1, 2017

Uh oh!

arielb1 commented Jun 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanna-kruppe commented Jun 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimNN commented Apr 6, 2017•
edited
Loading

luqmana commented Apr 7, 2017 via email •
edited
Loading

arielb1 commented May 2, 2017•
edited
Loading

arielb1 commented May 23, 2017•
edited
Loading

arielb1 commented Jun 6, 2017•
edited
Loading

hanna-kruppe commented Jun 6, 2017•
edited
Loading

arielb1 commented Jun 14, 2017•
edited
Loading