Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inference heuristic hacks #13355

Merged
merged 1 commit into from
Oct 23, 2015
Merged

Fix inference heuristic hacks #13355

merged 1 commit into from
Oct 23, 2015

Conversation

simonster
Copy link
Member

These were broken by #11274 but no one noticed. Fixes #13350.

@simonster
Copy link
Member Author

With this PR:

~ julia --depwarn=no perf_parens.jl 
Warmup @time
  0.000002 seconds (148 allocations: 10.151 KB)
Without parentheses:
  0.071398 seconds (15 allocations: 624 bytes)
With parentheses:
  0.073499 seconds (15 allocations: 624 bytes)

@tkelman
Copy link
Contributor

tkelman commented Sep 29, 2015

failing an llvm assertion on osx and windows?

Assertion failed: (isVirtualRegister(Reg) && "Not a virtual register"), function virtReg2Index, file /private/tmp/llvm33-julia-IYf1/llvm-3.3.src/include/llvm/Target/TargetRegisterInfo.h, line 293.
signal (6): Abort trap: 6
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
    From worker 3:       * functional           Worker 3 terminated.
ERROR (unhandled task failure): EOFError: read end of file

@timholy
Copy link
Member

timholy commented Sep 29, 2015

Thanks for looking into this, @simonster!

@simonster
Copy link
Member Author

The LLVM assertion failure breaks the tests with this PR but is possible to trigger without it. Filed #13366.

@KristofferC
Copy link
Member

Maybe it is possible to add a test for this?

@simonster
Copy link
Member Author

I think it would be non-trivial to test directly and the logic is pretty simple. But we should definitely have a perf test for #5011 or #13350. I'll try to add that to @IainNZ's collection.

@JeffBezanson
Copy link
Member

Excellent! Thanks for figuring this out.

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2015

So should I be cherry-picking this into #13357 if the assertion failure is a master-only issue?

simonster added a commit that referenced this pull request Sep 30, 2015
These were broken by #11274 but no one noticed. Fixes #13350.

(cherry picked from commit 7dfcf70)
ref #13355
@simonster
Copy link
Member Author

@tkelman Yes, I think so.

@JeffBezanson
Copy link
Member

Bump. I think we should rebase and merge this.

@simonster simonster force-pushed the sjk/inference-heuristics branch 2 times, most recently from d1e1fc3 to 9822e93 Compare October 22, 2015 19:10
These were broken by #11274 but no one noticed. Fixes #13350.
@simonster simonster force-pushed the sjk/inference-heuristics branch from 9822e93 to f8d2294 Compare October 22, 2015 19:33
JeffBezanson added a commit that referenced this pull request Oct 23, 2015
@JeffBezanson JeffBezanson merged commit bc0e2bb into master Oct 23, 2015
@tkelman tkelman deleted the sjk/inference-heuristics branch October 24, 2015 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants