[PATCH] These builtin functions set errno. Mark them accordingly.

Chad Rosier mcrosier at codeaurora.org
Mon May 19 09:26:28 PDT 2014


Hal and I discussed this on IRC and here's the synopsis (mostly in Hal's words):

Many of these builtins turn into libc calls, and those do set errno. Marking them as readnone is a problem, because we can reorder/eliminate writes to errno.  For example, if we assume readnone for something like __builtin_sqrt(); write(); perror(); it may be reordered to write(); __builtin_sqrt(); perror(); and perror may provide junk.  However, if we don't mark the builtins as readnone, then they won't get lowered into ISD nodes (for those nodes that are relevant), and that means that they won't get instruction selected into native instructions on targets where that's possible. So the __builtin_sqrt is kind of an escape hatch to force the direct lowering behavior even when we can't do it for sqrt because sqrt might set errno.  But, as Hal said, this behavior *is* broken, technically. And, in fact, it is not hard to create test cases where you can see buggy behavior.  Hak's just not sure what the right way of handling it is. Hal thinks we really need some kind of target input, so that we can know ahead of time whether the builtin will *really* set errno or not after isel.  Hal thinks there are two solutions possible. 

1. Like with inline asm registers/constraints; each target will provide a table describing what builtins will really not set errno.
2. Let Clang essentially query TTI for those builtins that are relevant. This second method will require a bit of infrastructure work, but is probably not too difficult.

I'm not going to push this patch further because it's not the right solution.  In theory, I believe it is correct, but it breaks the __buitlin escape hatch, which would likely result in serious performance degradations.

================
Comment at: include/clang/Basic/Builtins.def:165
@@ -166,1 +164,3 @@
+BUILTIN(__builtin_atanhf, "ff", "Fne")
+BUILTIN(__builtin_atanhl, "LdLd", "Fneqq")
 BUILTIN(__builtin_cbrt , "dd", "Fnc")
----------------
hfinkel at anl.gov wrote:
> qq?
That's must have slipped in.  Thanks, Hal.

http://reviews.llvm.org/D3806






More information about the cfe-commits mailing list