Out of curiosity, which of these commits is the actual fix for our issue?

The fourth patch is the one that nominally fixes it. I included the fifth one because it claims to have fixed a bug in the fourth one. With only this two commit the LLVM tests fails and the failure looks like real codegen regressions so I included a few more that touches the same tests and the LLVM tests are now passing. I'm not entirely sure which ones fixes the "regression" caused by the fix but from the commit message I suspect it's the third one.

Counted by the order they are cherry picked (see the [PATCH */5] in the commit message) Apparently I got the order wrong in llvm.mk but there doesn't seem to be too many conflicts.....

So the one that should be fixing this issue is [PATCH 4/5] Avoid false dependencies of undef machine operands (also the biggest one though a big part of it is tests...). The fix for it is [PATCH 5/5] Fixing bug committed in rev. 278321. The one that I think fixes the regression is [PATCH 3/5] ExecutionDepsFix - Fix bug in clearance calculation

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@nanosoldier runbenchmarks(ALL, vs = ":master")

Just to check which ones are noise....

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

looks like ["array","growth",("append!",2048)] was noise, the rest were not

I've looked into the regression and the problem is that LLVM fails to take into account the clearance from an incoming backedge when choosing which register to use. Should be fixable, I'll try for a patch.

Upstream patch is here: https://reviews.llvm.org/D28759

Will push an update to this PR that adds the upstream patch, so we can re-run nanosoldier.

@nanosoldier runbenchmarks(ALL, vs = ":master")

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

Looks like I did indeed fix the super bad regressions we saw. There's still comprehension_iteration left which I should probably look at. Also, there's some new ones which may or may not be real.
Let's run again to see: @nanosoldier runbenchmarks(ALL, vs = ":master")

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

Ok, I'm gonna go with the comprehension one is the only remaining regression. Looking into it.

["scalar","arithmetic",("div","Complex{Float32}","Complex{Float32}")] also looks real and a bit larger?

Ah, yes. In any case, the cause seems to be what @yuyichao mentioned to me. Namely, that calls to non-inlined functions don't reset clearance information.

https://reviews.llvm.org/D28786 same procedure yesterday.

@nanosoldier runbenchmarks(ALL, vs = ":master")

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

Travis fails on 32bit:

julia: /home/travis/build/JuliaLang/julia/deps/srccache/llvm-3.9.1/include/llvm/CodeGen/MachineOperand.h:268: unsigned int llvm::MachineOperand::getReg() const: Assertion `isReg() && "This is not a register operand!"' failed.

I was unable to reproduce this locally, but I had an off by one in the patch, which could have easily caused that. Fixed.

Also, I think the last nanosoldier run picked up some improvements on master, so @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier runbenchmarks(ALL, vs = ":master")

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

Nanosoldier seems very happy with this 🎉. LGTM if travis is green

The travis failure looks unrelated. Any ideas what that might be?

it's the mac spawn test issue that #20073 is attempting to fix. these patches should be updated to match whatever gets committed upstream once it goes through review there, but lgtm for now

Let's wait a few days while upstream review is ongoing. The problem with replacing these patches later is that they won't apply cleanly to people who've had this version applied. Also, FYI, the second commit has been split up into
https://reviews.llvm.org/D28915.

Any news upstream on these?

No, but it's only been one business day since I posted that, and some of these folks might not work weekends ;).

Any news a week later now?

Yes, for some reason I'm not getting email notifications on these anymore though.

I had the same happen on phabricator, seems flaky

1/3 is now nominally upstream. We'll see if it survives the buildbot onslaught.

Bump, any news?

@Keno what is the status on these?

Making its way through review upstream.

Can we rebase this to reflect the latest state and get that in before alpha, then (later) add additional incremental patches on top of that as the upstream review continues?

getting the latest state of this in would help make the benchmark resilts much more predictable and useful

@Keno can we please just merge the latest state of this patch set now?

The latest state of this patch is still very much in flux. We can however, rebase this PR as is and merge it.

one of them was upstreamed, wasn't it? what's closer to what will eventually be upstream, what's in this PR now or the current state of the upstream reviews?

OS X failure: https://gist.github.com/StefanKarpinski/1663878980f211580b7fb83b93d2e911

been a while since this last ran @nanosoldier runbenchmarks(ALL, vs = ":master")

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

Dang, that's a lot of performance improvements. Hard to believe LLVM let something this big regress. Oh wait, no it isn't.

We got the best LLVM hackers don't we?