[PATCH] D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible.

Jordan Rupprecht via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 21 14:47:31 PDT 2018


rupprecht added inline comments.


================
Comment at: lib/builtins/fp_lib.h:274
+// the __compiler_rt prefix.
+static __inline fp_t __compiler_rt_logbX(fp_t x) {
+  rep_t rep = toRep(x);
----------------
compnerd wrote:
> I don't see why we need `__inline`, I'm pretty sure that we compile in C99 mode, so `inline` should be fine.  Doesn't this require GNU semantics though?  Did you try building the windows target with this change?
This is consistent with all the other methods in this file.

Actually, it looks like the change from `inline` to `__inline` was from your revision: https://reviews.llvm.org/rL249953


================
Comment at: lib/builtins/fp_lib.h:302
+  }
+}
+#endif
----------------
compnerd wrote:
> Is this taken from somewhere else or is this code written from scratch?
This is written from scratch.


================
Comment at: lib/builtins/fp_lib.h:323
+}
+  #endif
 #endif
----------------
compnerd wrote:
> Why the fallback to `libm`?
> 
> Additionally, the names are different, so is there a reason to not just define the variants unconditionally?
> Why the fallback to libm?
This generic implementation only makes sense for ieee754, but some of the compiler-rt methods use other floating point types (e.g. divxc3 uses x86 80-bit long doubles). Hence, this implementation is incomplete and is using libm for now. (Note: this is not adding a dependency on libm, this is just calling out an existing one).

> 
> Additionally, the names are different, so is there a reason to not just define the variants unconditionally?
I originally had it split up like that (history link -- https://reviews.llvm.org/D49514?vs=on&id=156161&whitespace=ignore-most#toc -- things are split up into compiler_rt_logb.c etc.), but Eli suggested making it inline in an earlier comment. I think this way is a little nicer, but I don't have a strong opinion.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D49514





More information about the llvm-commits mailing list