Host-dependent (unstable) emitting SwitchInst

Quuxplusone commented 13 years ago


Bugzilla Link	PR9581
Status	REOPENED
Importance	P normal
Reported by	NAKAMURA Takumi (geek4civic@gmail.com)
Reported on	2011-03-29 10:13:47 -0700
Last modified on	2014-08-11 12:51:09 -0700
Version	2.9
Hardware	PC Windows NT
CC	anton@korobeynikov.info, dexonsmith@apple.com, geek4civic@gmail.com, llvm-bugs@lists.llvm.org, rnk@google.com
Fixed by commit(s)
Attachments	`foo.ll` (17529 bytes, application/octet-stream) `foo.a.s` (41499 bytes, text/plain) `foo.r.s` (41499 bytes, text/plain) `foo.c.s` (41499 bytes, text/plain)
Blocks	PR9591
Blocked by	PR6907
See also

Created attachment 6370
foo.ll: from BlackfinRegisterInfo.cpp

With g++.exe (TDM-1 mingw32) 4.4.0,

--enable-assertions affects behavior of code generator.
It seems floating point arithmetics would affect.

I can see identical outputs to suppress one debug print.

--- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -2035,11 +2035,13 @@ bool SelectionDAGBuilder::handleBTSplitSwitchCase(CaseRe
c& CR,
                            (Last - RBegin + 1ULL).roundToDouble();
     double Metric = Range.logBase2()*(LDensity+RDensity);
     // Should always split in some non-trivial place
+#if 0
     DEBUG(dbgs() <<"=>Step\n"
                  << "LEnd: " << LEnd << ", RBegin: " << RBegin << '\n'
                  << "LDensity: " << LDensity
                  << ", RDensity: " << RDensity << '\n'
                  << "Metric: " << Metric << '\n');
+#endif
     if (FMetric < Metric) {
       Pivot = J;
       FMetric = Metric;

Quuxplusone commented 13 years ago

Attached foo.ll (17529 bytes, application/octet-stream): foo.ll: from BlackfinRegisterInfo.cpp

Quuxplusone commented 13 years ago

Attached foo.a.s (41499 bytes, text/plain): Release+Asserts/bin/llc

Quuxplusone commented 13 years ago

Attached foo.r.s (41499 bytes, text/plain): Release/bin/llc

Quuxplusone commented 13 years ago

Attached foo.c.s (41499 bytes, text/plain): clang-Release/bin/llc

Quuxplusone commented 13 years ago

They are generated with "llc -O0".

Quuxplusone commented 13 years ago

Workaround:

--- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -2029,9 +2029,9 @@ bool
SelectionDAGBuilder::handleBTSplitSwitchCase(CaseRec& CR,
     APInt Range = ComputeRange(LEnd, RBegin);
     assert((Range - 2ULL).isNonNegative() &&
            "Invalid case distance");
-    double LDensity = (double)LSize.roundToDouble() /
+    volatile double LDensity = (double)LSize.roundToDouble() /
                            (LEnd - First + 1ULL).roundToDouble();
-    double RDensity = (double)RSize.roundToDouble() /
+    volatile double RDensity = (double)RSize.roundToDouble() /
                            (Last - RBegin + 1ULL).roundToDouble();
     double Metric = Range.logBase2()*(LDensity+RDensity);
     // Should always split in some non-trivial place

Quuxplusone commented 13 years ago

Resolved in r129199, thanks!

Quuxplusone commented 13 years ago

(In reply to comment #6)
> Resolved in r129199, thanks!

Thanks Chris! A little weird it is!

Quuxplusone commented 13 years ago

Between i686-mingw32-clang.exe and x86_64-mingw32-clang.exe, still precision
issue is there.

1st patch is;
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110411/119441.html
It can revert r129199.
I know it could be generalized.

Does anyone try to compare i686(w/o sse2) and x86_64 on other hosts?

Quuxplusone commented 10 years ago

Looks like this code still exists:
    // Use volatile double here to avoid excess precision issues on some hosts,
    // e.g. that use 80-bit X87 registers.
    volatile double LDensity =
       (double)LSize.roundToDouble() /
                           (LEnd - First + 1ULL).roundToDouble();
    volatile double RDensity =
      (double)RSize.roundToDouble() /
                           (Last - RBegin + 1ULL).roundToDouble();
    volatile double Metric = Range.logBase2()*(LDensity+RDensity);

Can it be fixed with ScaledNumber?

Quuxplusone / LLVMBugzillaTest

Host-dependent (unstable) emitting SwitchInst #7822