[builtins] Implement rounding mode support for i386/x86_64

[nickdesaulniers]

compiler-rt/lib/builtins/i386/fp_mode.c

19

This could be a local const uint32_t (local to __fe_raise_inexact).

43–45

switch (cw & X87_RMODE_MASK). LLVM will optimize such expressions in a switch.

66

Should you use env.__mxcsr here? If so, then the preprocessor guard differs from the above.

69

Do *any* of these asm statements in this patch require volatile qualification?

[craig.topper]

compiler-rt/lib/builtins/i386/fp_mode.c

43–45

By the time its gets to IR and goes through mem2reg/SROA it should look the same right?

[nickdesaulniers]

compiler-rt/lib/builtins/i386/fp_mode.c

43–45

Yep. Just slightly more concise.

[kongyi]

compiler-rt/lib/builtins/i386/fp_mode.c

66

No, these are different. One is FPU environment and this one is for the SSE unit.

69

Yes, otherwise the entire code block may be optimised away.

[kongyi]

gentle ping...

[craig.topper]

compiler-rt/lib/builtins/i386/fp_mode.c

62

Why do we need fnclex here?

63

Do we need an fwait after this to trigger the exception interrupt?

[kongyi]

compiler-rt/lib/builtins/i386/fp_mode.c

62

Android's feraiseexecept has this, but it is indeed unnecessary. Removed.

63

Yes. Added.

[nickdesaulniers]

compiler-rt/lib/builtins/i386/fp_mode.c

38

It seems that fenv_t would be defined in <cfenv>, can you include that instead of redefining it?
http://www.cplusplus.com/reference/cfenv/fenv_t/

Looks like all of the above constants are defined as enums there, too.

[craig.topper]

Would it be better to just do a dummy inexact division operation to trigger than inexact exception instead of manipulating the environment?

[kongyi]

In D69870#1744626, @craig.topper wrote:

Would it be better to just do a dummy inexact division operation to trigger than inexact exception instead of manipulating the environment?

I assumed this would have better performance than dummy inexact division operation. Do you want me to verify it?

[craig.topper]

In D69870#1744853, @kongyi wrote:

In D69870#1744626, @craig.topper wrote:

Would it be better to just do a dummy inexact division operation to trigger than inexact exception instead of manipulating the environment?

I assumed this would have better performance than dummy inexact division operation. Do you want me to verify it?

In D69870#1744853, @kongyi wrote:

In D69870#1744626, @craig.topper wrote:

Would it be better to just do a dummy inexact division operation to trigger than inexact exception instead of manipulating the environment?

I assumed this would have better performance than dummy inexact division operation. Do you want me to verify it?

I can't imagine fldenv/fstenv are very fast instructions. I wouldn't be surprised if they serialized the core and prevented speculation past them.

[kongyi]

Remove unused include

[nickdesaulniers]

No more nits from me. Please wait for LGTM from ctopper for functional review.

[craig.topper]

LGTM

[thakis]

compiler-rt/lib/builtins/CMakeLists.txt

242

It looks a bit unusual that a file that works on both 32-bit and 64-bit x86 is in the i386 folder. All other files in the i386 folder are 32-bit only as far as I can tell.

[kongyi]

compiler-rt/lib/builtins/CMakeLists.txt

242

I'm working on this, https://reviews.llvm.org/D69688

[hans]

compiler-rt/lib/builtins/i386/fp_mode.c

21

This won't work with MSVC. See e.g. http://lab.llvm.org:8011/builders/sanitizer-windows/builds/54305

I've reverted in a19f0eec94e.

compiler-rt/lib/builtins/CMakeLists.txt

set(x86_ARCH_SOURCES
   fixunsxfsi.c
   fixunsxfti.c
   floatdixf.c
   floattixf.c
   floatundixf.c
   floatuntixf.c
   mulxc3.c
   powixf2.c
+  i386/fp_mode.c
 )
 
 if (NOT MSVC)
   set(x86_64_SOURCES
     ${GENERIC_TF_SOURCES}
     x86_64/floatdidf.c
     x86_64/floatdisf.c
     x86_64/floatdixf.c

compiler-rt/lib/builtins/i386/fp_mode.c

+//===----- lib/i386/fp_mode.c - Floaing-point mode utilities -----*- C -*-====//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <stdint.h>
+
+#include "../fp_mode.h"
+
+#define X87_TONEAREST  0x0000
+#define X87_DOWNWARD   0x0400
+#define X87_UPWARD     0x0800
+#define X87_TOWARDZERO 0x0c00
+#define X87_RMODE_MASK (X87_TONEAREST | X87_UPWARD | X87_DOWNWARD | X87_TOWARDZERO)
+
+#define X87_INEXACT    0x20
+
+typedef struct {
+  unsigned short __control_word;
+  unsigned short __unused1;
+  unsigned short __status_word;
+  unsigned short __unused2;
+  unsigned short __tags;
+  unsigned short __unused3;
+  unsigned int   __eip;
+  unsigned short __cs_selector;
+  unsigned short __opcode;
+  unsigned int   __data_offset;
+  unsigned short __data_selector;
+  unsigned short __unused5;
+#ifdef __X86_64__
+  unsigned int   __mxcsr;
+#endif
+} fenv_t;
+
+FE_ROUND_MODE __fe_getround() {
+  // Assume that the rounding mode state for the fpu agrees with the SSE unit.
+  unsigned short cw;
+  __asm__ __volatile__ ("fnstcw %0" : "=m" (cw));
+  cw &= X87_RMODE_MASK;
+
+  switch (cw) {
+    case X87_TONEAREST:
+      return FE_TONEAREST;
+    case X87_DOWNWARD:
+      return FE_DOWNWARD;
+    case X87_UPWARD:
+      return FE_UPWARD;
+    case X87_TOWARDZERO:
+      return FE_TOWARDZERO;
+  }
+  return FE_TONEAREST;
+}
+
+int __fe_raise_inexact() {
+  fenv_t env;
+  __asm__ __volatile__ ("fnstenv %0": "=m" (env));
+  env.__status_word |= X87_INEXACT;
+  __asm__ __volatile__ ("fnclex");
+  __asm__ __volatile__ ("fldenv %0": : "m" (env));
+
+#ifdef __SSE_MATH__
+  uint32_t mxcsr;
+  __asm__ __volatile__ ("stmxcsr %0": "=m" (mxcsr));
+  mxcsr |= X87_INEXACT;
+  __asm__ __volatile__ ("ldmxcsr %0": : "m" (mxcsr));
+#endif
+  return 0;
+}

compiler-rt/test/builtins/Unit/addtf3_test.c

if (test__addtf3(makeInf128(),
         return 1;
     // any + any
     if (test__addtf3(0x1.23456734245345543849abcdefp+5L,
                      0x1.edcba52449872455634654321fp-1L,
                      UINT64_C(0x40042afc95c8b579),
                      UINT64_C(0x61e58dd6c51eb77c)))
         return 1;
 
-#if (defined(__arm__) || defined(__aarch64__)) && defined(__ARM_FP)
+#if (defined(__arm__) || defined(__aarch64__)) && defined(__ARM_FP) || \
+    defined(i386) || defined(__x86_64__)
     // Rounding mode tests on supported architectures
     const long double m = 1234.0L, n = 0.01L;
 
     fesetround(FE_UPWARD);
     if (test__addtf3(m, n,
                      UINT64_C(0x40093480a3d70a3d),
                      UINT64_C(0x70a3d70a3d70a3d8)))
         return 1;

compiler-rt/test/builtins/Unit/subtf3_test.c

if (test__subtf3(makeInf128(),
         return 1;
     // any - any
     if (test__subtf3(0x1.234567829a3bcdef5678ade36734p+5L,
                      0x1.ee9d7c52354a6936ab8d7654321fp-1L,
                      UINT64_C(0x40041b8af1915166),
                      UINT64_C(0xa44a7bca780a166c)))
         return 1;
 
-#if (defined(__arm__) || defined(__aarch64__)) && defined(__ARM_FP)
+#if (defined(__arm__) || defined(__aarch64__)) && defined(__ARM_FP) || \
+    defined(i386) || defined(__x86_64__)
     // Rounding mode tests on supported architectures
     const long double m = 1234.02L, n = 0.01L;
 
     fesetround(FE_UPWARD);
     if (test__subtf3(m, n,
                      UINT64_C(0x40093480a3d70a3d),
                      UINT64_C(0x70a3d70a3d70a3d7)))
         return 1;