From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.2 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by inbox.vuxu.org (OpenSMTPD) with SMTP id 65dfbe93 for ; Fri, 24 Jan 2020 04:13:49 +0000 (UTC) Received: (qmail 27939 invoked by uid 550); 24 Jan 2020 04:13:47 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 27921 invoked from network); 24 Jan 2020 04:13:45 -0000 X-Authentication-Warning: key0.esi.com.au: damianm owned process doing -bs Date: Fri, 24 Jan 2020 15:13:32 +1100 (AEDT) From: Damian McGuckin To: musl@lists.openwall.com In-Reply-To: <20200124011122.GP30412@brightrain.aerifal.cx> Message-ID: References: <20200116161427.GO30412@brightrain.aerifal.cx> <20200116193343.GP30412@brightrain.aerifal.cx> <20200117164143.GB2020@voyager> <20200118011535.GD23985@port70.net> <20200118053759.GX30412@brightrain.aerifal.cx> <20200118094015.GE23985@port70.net> <20200124011122.GP30412@brightrain.aerifal.cx> User-Agent: Alpine 2.02 (LRH 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII Subject: Re: [musl] Considering x86-64 fenv.s to C Let me know if I missed something in the following: On Thu, 23 Jan 2020, Rich Felker wrote: >> #ifndef FE_ALL_EXCEPT >> #define FE_VALID_EXCEPT (FE_INEXACT|FE_DIV_BY_ZERO|FE_UNDERFLOW|FE_OVERFLOW) >> #define FE_ALL_EXCEPT (FE_INVALID|FE_VALID_EXCEPT) >> #endif > > I don't understand what this is for. FE_ALL_EXCEPT is defined in the > public fenv.h and FE_VALID_EXCEPT is not used. For the first question, it was done purely to avoid the need to define FE_ALL_EXCEPT in all of the various 'fenv.h' files. I am happy to throw it away. For the second question, at one stage, I considered having the concept of what was anything except an INVALID exception. I will remove those 4 lines. >> /* >> * The usual approach to validating an exception mask is to throw out >> * anything which is illegal. Some systems will NOT do this, choosing >> * instead to return a non-zero result on encountering an illegal mask >> */ > > The latter is not desirable behavior and not something you need to be > trying to reproduce. It's a bug or at least an unspecified case we > don't care about in existing code. Wonderful. I was purely trying to preserve historical performance. I am more than happy to NOT do this. That makes the next problem easy to fix. >> #ifndef FE_VALIDATE_EXCEPT >> #define FE_VALIDATE_EXCEPT(e) e &= FE_ALL_EXCEPT >> #endif > > Write things like this directly in code; don't hide them behind > macros. Doing that just makes the code hard to read because you can't > tell what it's doing without searching for the macro definition. (Also > the macro is not protected by parens as written but that's easily > fixable.) Because we can ignore that non-zero return case, we no longer need an architecture dependent macro for this so it can go directly in the code. So, again, an easy problem to fix. >> /* >> * There is usually no need to mess the generic valid exceptions bits >> * given to 'feclearexcept' and 'feraiseexcept' so define a 'nothing' >> * macro for such a scenario - is this the MUSL accepted style???? >> */ >> #ifndef FE_QUALIFY_CLEAR_EXCEPT >> #define FE_QUALIFY_CLEAR_EXCEPT(excepts) (0) >> #endif >> #ifndef FE_QUALIFY_RAISE_EXCEPT >> #define FE_QUALIFY_RAISE_EXCEPT(excepts) (0) >> #endif > > Can you clarify how these would be defined for powerpc? Could the logic > just be embedded in get_sr/set_sr if needed? Address the second question first, my original aim was to keep any logic out of where embedded assembler was used, mainly because of my own very irregular experience with it. I could also not convince myself that get_sr and set_sr may need to know from where they are called. In the case of the powerpc64, with the SR and CR being the same, that adds to the complexity, so I gave up. Answering your first question, for the powerpc64. #define QUALIFY_CLEAR_EXCEPT(e) if (e & FE_INVALID) e |= FE_ALL_INVALID #define QUALIFY_RAISE_EXCEPT(e) if (e & FE_INVALID) e |= FE_INVALID_SOFTWARE An alternative is to avoid these macros and then #ifndef FE_ALL_INVALID #define FE_ALL_INVALID 0 #endif #ifndef FE_INVALID_SOFTWARE #define FE_INVALID_SOFTWARE 0 #endif and have respectively the following code in feclear.. and feraise.. if (excepts & FE_INVALID) excepts |= FE_ALL_INVALID; and if (excepts & FE_INVALID) excepts |= FE_INVALID_SOFTWARE; An optimize should see the OR with zero for most architectures and then ignore that whole 'if' statement because the action was a NO-OP. > >> /* >> * Compute the rounding mask with some rigid logic >> */ >> #ifndef FE_TO_NEAREST >> #define FE_TO_NEAREST 0 >> #define ROUND_MASK ((unsigned int) 0) >> #else >> #ifndef FE_TOWARDS_ZERO >> #define ROUND_MASK ((unsigned int) 0) >> #else >> #ifdef FE_DOWNWARD >> #ifdef FE_UPWARD >> #define ROUND_MASK ((unsigned int) (FE_DOWNWARD|FE_UPWARD|FE_TOWARDZERO)) >> #else >> #define ROUND_MASK UNSUPPORTED rounding >> #endif >> #else >> #ifdef FE_UPWARD >> #define ROUND_MASK UNSUPPORTED rounding >> #else >> #define ROUND_MASK ((unsigned int) (FETOWARDZERO)) >> #endif >> #endif >> #endif >> #endif > > I don't see why you have "UNSUPPORTED rounding" cases here. Any > combination should be valid. My deliberate assumption was that the only legal combinations were round to nearest only or round to nearest and truncate only or round to nearest, truncate, and round to either +/- INFINITY Whether that assumption is valid is another thing but I am happy to be proven wrong. If somebody defined an architecture where that assumption was not valid, I wanted the compiler to complain at me. > Since these are not bitmasks, probably the arch should just have to > define the rounding mode mask. For example if the values were 0b000, > 0b001, 0b100, and 0b101, but the 0 in the middle was significant, it > should probably be treated as part of the mask. That was not the way I read it but again, I am happy to be proven wrong. If the net OR'ing of the various types of rounding cannot be treated as a mask, then every *BSD libc implementations that I know will need to be fixed because they do treat it that way. I did run tests and none of the ones I tested from BSD and MUSL had that structure. Maybe my test code was wrong. They all seems to be 2 bits where TO_NEAREST was all bits zero and one of the others was the some of the other two. > If you really want to generate the mask, something like > > #ifdef FE_TONEAREST > #define FE_TONEAREST_MASK FE_TONEAREST > #else > #define FE_TONEAREST_MASK 0 > #endif > ... > #define ROUND_MASK (FE_TONEAREST_MASK | FE_DOWNWARD_MASK | ...) > > should be simpler and comprehensive. I will run a test but I think they produce the same result for all the architecture for which I have definition files. >> int >> feclearexcept(excepts) >> { >> FE_VALIDATE_EXCEPT(excepts); >> FE_QUALIFY_CLEAR_EXCEPT(excepts); >> setsr(getsr() & ~((unsigned int) excepts)); >> return(0); >> } > > No function-like parens around return value, no gratuitous casts. No problems. Sorry, gratuitous casting is a habit to stop various linters complaining. I went looking for a MUSL style guide but could not find one. >> static inline int >> __raisearithmetically(int excepts) >> { >> /* >> * assume single OP is faster than double OP >> */ >> const float one = (float) 1; >> const float zero = (float) 0; >> const float tiny = (float) 0x1.0p-126; >> const float huge = (float) 0x1.0p+126; >> volatile float x; >> >> /* >> * if it is just a simple exception, arithmetic expressions are optimal >> */ >> switch(excepts) >> { >> case FE_INVALID: >> x = zero, x /= x; >> break; >> case FE_DIVBYZERO: >> x = zero, x = one / x; >> break; >> case FE_INEXACT: >> x = tiny, x += one; >> break; >> case (FE_OVERFLOW | FE_INEXACT): >> x = huge, x *= x; >> break; >> case (FE_UNDERFLOW | FE_INEXACT): >> x = tiny, x *= x; >> break; >> default: /* if more than one exception exists, a sledgehammer is viable */ >> setsr(getsr() | ((unsigned int) excepts)); >> break; >> } >> return(0); >> } > > This is probably ok (aside from style things mentioned above), but > simply recording the flags in software without writing them to the > status register at all may be preferable. In that case we would not even > need a set_sr primitive, only get_sr and clear_sr. Yes. I like the idea. But I was deliberately not trying to change anything external to 'fenv.c'. Also, is the handling of __pthread_self() protected into the future? > Doing this would also simplify the ppc special-casing for FE_INVALID I > think.. I think only for feraiseexcept as you show. It does not simplify feclearexcept as far as I can see. >> int >> feraiseexcept(int excepts) >> { >> FE_VALIDATE_EXCEPT(excepts); >> FE_QUALIFY_RAISE_EXCEPT(excepts); >> return __raisearithmetically(excepts); >> } > > Then this could just be __pthread_self()->fpsr |= excepts; Very clean and simple. Is it portable outside of Linux? Not that this is relevant for MUSL which is designed for Linux-based devices but I do work on non-Linux (and no C/C++) systems so I am curious. >> int >> fegetround(void) >> { >> return (int) (getcr() & ROUND_MASK); >> } >> >> int >> fesetround(int rounding_mode) >> { >> if ((rounding_mode & ~ROUND_MASK) == 0) >> { >> unsigned int mode = ((unsigned int) rounding_mode); >> >> return (setcr((getcr() & ~ROUND_MASK) | (mode & ROUND_MASK)), 0); >> } >> return(-1); >> } > > I would put the error test at the beginning rather than putting the > whole body inside {}: > > if (rounding_mode & ~ROUND_MASK) return -1; Happy to. A while ago, I found that the alternative produces more optimal code when dealing with floating point dominant routines. But that is huge generalization and I have not tested that generalization in this case. >> int >> fegetenv(fenv_t *envp) >> { >> return ((gete(envp)), 0); >> } > > Is there a reason for using comma operator here? I used it to let the compiler prove that my #defines for gete() and sete() worked as if they were a single statement. That gave me some rigour on the design of that macro, which in the case of the m68k (and eventually the i386), does demand a comma operator. But for the production code, it will disappear. >> int >> fesetenv(fenv_t *envp) >> { >> fenv_t envpd = FE_DFL_ENV_DATA, *e = envp == FE_DFL_ENV ? &envpd : envp; >> >> return ((sete(envp)), 0); >> } > > It might be preferable to use a static const object for default env; > I'm not sure. Not a big deal either way though. I thought of that. I used to always do that until 5 years ago until I found that an auto variable would often produce more optimal code. It needs to be checked but it is irrelevant to the fundament design issues. Ongoing: I will incorporate your suggestions except for the __pthread_self()->fpsr tweaks which I like but cannot test across multiple O/S's and then we can review this. Should I repost to make sure I got everything and we have a reference point? I will post some inline assembler for your review and entertainment but in the spirit of a generic template, they should be reviewed in isolation. Also, I notice that on the mips, the single unsigned in an 'fenv_t' is encapsulated in a struct. Is this historical, mandatory, stylistic, a way to fix a compiler bug, compatability or something else? I also notice that there is a subtle difference in the 'fegetenv' assembler between diff mips/*.S mips64/*.S 64c64 < addiu $5, $4, 1 --- > daddiu $5, $4, 1 I guess this is just struct alignment issues? Sorry, it is 20 years since I worked with the assembler on a MIPS so my knowledge is really rusty. Regards - Damian Pacific Engineering Systems International, 277-279 Broadway, Glebe NSW 2037 Ph:+61-2-8571-0847 .. Fx:+61-2-9692-9623 | unsolicited email not wanted here Views & opinions here are mine and not those of any past or present employer