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

LLVM 3.6 giving incorrectly signed zeros #10377

Closed
simonbyrne opened this issue Mar 2, 2015 · 22 comments · Fixed by #10424
Closed

LLVM 3.6 giving incorrectly signed zeros #10377

simonbyrne opened this issue Mar 2, 2015 · 22 comments · Fixed by #10424
Labels
bug Indicates an unexpected problem or unintended behavior upstream The issue is with an upstream dependency, e.g. LLVM

Comments

@simonbyrne
Copy link
Contributor

Using LLVM 3.6, branches involving comparison to zero can sometimes return the incorrect sign.

This has come up in #9336 (comment) and #9880 (comment).

Here is a minimal example:

julia> versioninfo()
Julia Version 0.4.0-dev+3639
Commit 7f7e9ae (2015-03-01 22:49 UTC)
Platform Info:
  System: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Xeon(R) CPU E7- 8850  @ 2.00GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Nehalem)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.6.0

julia> function foo(x,y)
           if x==y==0
               return y
           end
           y+1
       end
foo (generic function with 1 method)

julia> foo(0.0,-0.0)
0.0

And the @code_llvm foo(0.0,-0.0):

define double @julia_foo_42667(double, double) {
top:
  call void @llvm.dbg.value(metadata double %0, i64 0, metadata !10, metadata !16)
  call void @llvm.dbg.value(metadata double %1, i64 0, metadata !17, metadata !16)
  %2 = fcmp une double %0, %1, !dbg !18
  br i1 %2, label %L3, label %L1, !dbg !18

L1:                                               ; preds = %top
  call void @llvm.dbg.value(metadata double 0.000000e+00, i64 0, metadata !20, metadata !16)
  %phitmp = fcmp une double %0, 0.000000e+00, !dbg !18
  br i1 %phitmp, label %L3, label %if2, !dbg !18

if2:                                              ; preds = %L1
  ret double %0, !dbg !21

L3:                                               ; preds = %top, %L1
  %3 = fadd double %1, 1.000000e+00, !dbg !22
  ret double %3, !dbg !22
}

The problem here is that the if2 label it is returning %0 instead of %1.

@vtjnash vtjnash mentioned this issue Mar 2, 2015
19 tasks
@ivarne ivarne added the upstream The issue is with an upstream dependency, e.g. LLVM label Mar 2, 2015
@ViralBShah
Copy link
Member

Should we have a label for llvm bugs?

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2015

We can probably rename the "fixed in LLVM 3.5" label to be a general "LLVM issue" instead... or just keep using "upstream" where it's usually pretty clear which upstream is being referred to.

@simonbyrne
Copy link
Contributor Author

BTW, I haven't filed an upstream issue, as I wasn't really sure where the problem lay. Could someone who understands more about the internals help me out?

@JeffBezanson
Copy link
Sponsor Member

"fixed in LLVM 3.5" is actually a pretty useful label since we'll know exactly which issues to confirm and close when we upgrade LLVM.

@tkelman
Copy link
Contributor

tkelman commented Mar 3, 2015

"fixed in LLVM 3.5" is actually a pretty useful label since we'll know exactly which issues to confirm and close when we upgrade LLVM

True, but we can re-appropriate it after that's done. Either "fixed in LLVM 3.7" or something else.

@kmsquire
Copy link
Member

kmsquire commented Mar 3, 2015

Or just add a new label.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2015

BTW, I haven't filed an upstream issue, as I wasn't really sure where the problem lay. Could someone who understands more about the internals help me out?

I don't really understand the internals very well, but I think the way to communicate these things upstream would be to try to create a standalone .ll file with IR that returns incorrect results on 3.6.0 but correct on 3.5.1?

@simonbyrne
Copy link
Contributor Author

The problem is that the generated LLVM IR is incorrect (see above), so I'm not sure where the actual problem lies.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2015

Hm, then the upstream label is probably wrong.

@mbauman
Copy link
Sponsor Member

mbauman commented Mar 6, 2015

Isn't it just likely to be an LLVM optimization pass bug? Perhaps try disabling all the optimizations and see if it persists.

@ViralBShah ViralBShah added the bug Indicates an unexpected problem or unintended behavior label Mar 6, 2015
@simonbyrne
Copy link
Contributor Author

Looking at the debug info, here's the two optimisation passes between which the wrong value gets returned:

*** IR Dump After Combine redundant instructions ***
; Function Attrs: sspreq
define double @julia_foo_64477(double, double) #0 {
top:
  call void @llvm.dbg.value(metadata double %0, i64 0, metadata !10, metadata !16)
  call void @llvm.dbg.value(metadata double %1, i64 0, metadata !17, metadata !16)
  %2 = fcmp une double %0, %1, !dbg !18
  br i1 %2, label %L3, label %L1, !dbg !18

L1:                                               ; preds = %top
  call void @llvm.dbg.value(metadata double 0.000000e+00, i64 0, metadata !20, metadata !16)
  %phitmp = fcmp une double %1, 0.000000e+00, !dbg !18
  br i1 %phitmp, label %L3, label %if2, !dbg !18

if2:                                              ; preds = %L1
  ret double %1, !dbg !21

L3:                                               ; preds = %top, %L1
  %3 = fadd double %1, 1.000000e+00, !dbg !22
  ret double %3, !dbg !22
}
*** IR Dump After Global Value Numbering ***
; Function Attrs: sspreq
define double @julia_foo_64477(double, double) #0 {
top:
  call void @llvm.dbg.value(metadata double %0, i64 0, metadata !10, metadata !16)
  call void @llvm.dbg.value(metadata double %1, i64 0, metadata !17, metadata !16)
  %2 = fcmp une double %0, %1, !dbg !18
  br i1 %2, label %L3, label %L1, !dbg !18

L1:                                               ; preds = %top
  call void @llvm.dbg.value(metadata double 0.000000e+00, i64 0, metadata !20, metadata !16)
  %phitmp = fcmp une double %0, 0.000000e+00, !dbg !18
  br i1 %phitmp, label %L3, label %if2, !dbg !18

if2:                                              ; preds = %L1
  ret double %0, !dbg !21

L3:                                               ; preds = %top, %L1
  %3 = fadd double %1, 1.000000e+00, !dbg !22
  ret double %3, !dbg !22
}

@simonbyrne
Copy link
Contributor Author

Bug report opened here: LLVM #22823.

@simonbyrne
Copy link
Contributor Author

So it looks like it has been patched in svn (http://reviews.llvm.org/rL230564), but it just missed out on 3.6. Does this mean we need a "fixed in LLVM 3.7" tag?

@nalimilan
Copy link
Member

Or hopefully "to be fixed in LLVM 3.6.1"?

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2015

Ask in the bug thread / mailing list if the fix can be a candidate for backporting for 3.6.1.

edit: looks like you already did, great - see http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-March/082945.html for the patch release plans. We could try making a local patch for this too.

@StefanKarpinski
Copy link
Sponsor Member

LLVM doesn't do point releases.

@simonster
Copy link
Member

Don't they? There are point releases (3.4.1, 3.4.2, and 3.5.1) on the download page.

@StefanKarpinski
Copy link
Sponsor Member

Oh, I guess they started doing point releases in the 3.4 cycle. Nevermind, I hadn't noticed that.

@eschnett
Copy link
Contributor

eschnett commented Mar 6, 2015

3.6.1 is scheduled for March (this month), so backporting would need to happen soon.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2015

@eschnett see http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-March/082945.html, 3.5.2 is scheduled for March, 3.6.1 is scheduled for May. I just tested fixing this as a local patch and it seems to work, I'll open a PR.

@simonbyrne
Copy link
Contributor Author

This patch made the 3.6.1 release:
llvm-mirror/llvm@0ee7686

@tkelman
Copy link
Contributor

tkelman commented May 5, 2015

Awesome, thanks for keeping an eye on that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior upstream The issue is with an upstream dependency, e.g. LLVM
Projects
None yet
Development

Successfully merging a pull request may close this issue.