From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17116 invoked by alias); 23 Dec 2008 22:00:10 -0000 Received: (qmail 16842 invoked by uid 22791); 23 Dec 2008 22:00:07 -0000 X-SWARE-Spam-Status: No, hits=1.6 required=5.0 tests=AWL,BAYES_20,J_CHICKENPOX_32,J_CHICKENPOX_33,J_CHICKENPOX_41,J_CHICKENPOX_42,J_CHICKENPOX_51,J_CHICKENPOX_53,J_CHICKENPOX_61,J_CHICKENPOX_63,J_CHICKENPOX_73,J_CHICKENPOX_83,SPF_PASS X-Spam-Check-By: sourceware.org Received: from miranda.se.axis.com (HELO miranda.se.axis.com) (193.13.178.8) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 23 Dec 2008 21:59:36 +0000 Received: from ignucius.se.axis.com (ignucius.se.axis.com [10.13.11.50]) by miranda.se.axis.com (8.13.4/8.13.4/Debian-3sarge3) with ESMTP id mBNLxoCA023362; Tue, 23 Dec 2008 22:59:50 +0100 Received: from ignucius.se.axis.com (localhost [127.0.0.1]) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) with ESMTP id mBNLxDJ9004462; Tue, 23 Dec 2008 22:59:13 +0100 Received: (from hp@localhost) by ignucius.se.axis.com (8.12.8p1/8.12.8/Debian-2woody1) id mBNLxC9d004458; Tue, 23 Dec 2008 22:59:12 +0100 Date: Tue, 23 Dec 2008 22:00:00 -0000 Message-Id: <200812232159.mBNLxC9d004458@ignucius.se.axis.com> From: Hans-Peter Nilsson To: brolley@redhat.com, gdb-patches@sourceware.org, cgen@sourceware.org, binutils@sourceware.org Subject: [RFA:] CGEN sim operation extension fix; frv adjustment needed. MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Mailing-List: contact cgen-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cgen-owner@sourceware.org X-SW-Source: 2008-q4/txt/msg00020.txt.bz2 (CGEN for the stricter semantics, gdb-patches for containing a sim patch, binutils for being the project "owning" src/cpu.) Recent gcc (for example, gcc-4.3) has started to optimize for signed operations having undefined overflow semantics (i.e. it validly optimizes for the overflow not happening, a restricted domain of the input operands; please don't question the validity of the optimization here, go see the gcc archives if you're interested). For both cris-elf and frv-elf on a i686-pc-linux-gnu host with 4.3.0, you get the FAILs as below, because it gets problematic to check e.g. the sign-bit of the result of: (mul DI (zext siop1) (zext siop2)) which, as you know, is 1 for siop1=0xffffffff siop2=0xffffffff... Also, suddenly it's much harder to make (abs SI 0x80000000) stay 0x80000000 and has bit 31 on. cris-elf: Running /home/hp/sim/src/sim/testsuite/sim/cris/asm/asm.exp ... FAIL: crisv10 mulx.ms (execution) FAIL: crisv32 abs.ms (execution) FAIL: crisv32 mulx.ms (execution) frv-elf: Running /home/hp/sim/src/sim/testsuite/sim/frv/allinsn.exp ... FAIL: frv umulcc.cgs (execution) FAIL: fr500 umulcc.cgs (execution) FAIL: fr550 umulcc.cgs (execution) FAIL: fr400 umulcc.cgs (execution) FAIL: fr405 umulcc.cgs (execution) FAIL: fr450 umulcc.cgs (execution) FAIL: frv umulicc.cgs (execution) FAIL: fr500 umulicc.cgs (execution) FAIL: fr550 umulicc.cgs (execution) FAIL: fr400 umulicc.cgs (execution) FAIL: fr405 umulicc.cgs (execution) FAIL: fr450 umulicc.cgs (execution) CGEN operations are supposed to be sign-agnostic and are supposed to handle all bit-values as input(!), so to yield defined consistent and testable results, it should perform the operations internally using unsigned operands, which has the desired (and defined) wrap-semantics. In order to be consistent with the signedness of e.g. temporary variables, this requires casting the values back to the original type (that is, signed). So, besides accommodating current optimizations, it's an improvement in consistency. Currently, the signedness of the result of an operation is C-ish due to the operator implementation in cgen-ops.h. Because cgen allows sloppy^wimplicit size conversion (no explicit ext/zext required), the result depends on the type of the operands, which are implicit in the case of literal numbers. In an operation like: (add HI op1 65535) the operands are promoted to int and the signedness carries from that. This means that if you assign the result as in e.g.: (sequence ((DI tmp) (HI tmp2)) (set tmp (add HI op1 65535))) you'll get positive results for all positive op1 - as opposed to assigning it first to a temporary variable: (sequence ((DI tmp) (HI tmp2)) (set tmp2 (add HI op1 65535)) (set tmp tmp2)) I really don't think that's how it's intended to be. Unfortunately, the frv sim depends on the current inconsistency in it's comparisons for saturation. For example, with *only* the cgen-ops.h patch below only, you'd see: Running /tmp/hpautotest-sim/src/sim/testsuite/sim/frv/allinsn.exp ... FAIL: frv cmaddhss.cgs (execution) FAIL: fr500 cmaddhss.cgs (execution) FAIL: fr400 cmaddhss.cgs (execution) FAIL: frv cmaddhus.cgs (execution) FAIL: fr500 cmaddhus.cgs (execution) FAIL: fr400 cmaddhus.cgs (execution) FAIL: frv cmsubhss.cgs (execution) FAIL: fr500 cmsubhss.cgs (execution) FAIL: fr400 cmsubhss.cgs (execution) FAIL: frv cmsubhus.cgs (execution) FAIL: fr500 cmsubhus.cgs (execution) FAIL: fr400 cmsubhus.cgs (execution) FAIL: fr400 mabshs.cgs (execution) FAIL: frv maddhss.cgs (execution) FAIL: fr500 maddhss.cgs (execution) FAIL: fr400 maddhss.cgs (execution) FAIL: frv maddhus.cgs (execution) FAIL: fr500 maddhus.cgs (execution) FAIL: fr400 maddhus.cgs (execution) FAIL: frv msubhss.cgs (execution) FAIL: fr500 msubhss.cgs (execution) FAIL: fr400 msubhss.cgs (execution) FAIL: frv msubhus.cgs (execution) FAIL: fr500 msubhus.cgs (execution) FAIL: fr400 msubhus.cgs (execution) FAIL: frv mtrap.cgs (execution) FAIL: fr500 mtrap.cgs (execution) FAIL: fr400 mtrap.cgs (execution) (more errors elided) Changing to extend the operands explicitly helps. These FSF sims are CGEN-generated: cris, frv, iq2000, m32r, sh64. All but iq2000 have testsuites, supposedly covering edge cases like the ones we're talking about here. The iq2000 has none. I reviewed the iq2000 port which at a glance seems pretty trivial and not affected by this bug. For the record, ado162 and ado16 looks like they have a sign-extension-related bug; consider what happens in the last "or" operation when "low" has bit 15 set. You'd get 0xffff in the upper 16 bits, right? Still, that's an implicit sign-extension from a local variable, not from an operation, hence unaffected by this patch. No regressions with the patches below for any of the other sims. I've started gcc regression tests for frv, m32r and sh64 to raise confidence and will make sure there are no regressions before committing. Anyway, over to the patches. First, there's the frv.cpu patch, making the implicit promotions explicit and the result defined and as expected, independently of the cgen-ops.h patch. There are other ways, like assigning through a temporary of the correct mode or to pass the extend operator as a parameter or maybe the maitainer has another preferred route. In any case, I don't have commit authority over src/cpu which is defined as belonging to the binutils domain, so... Ok to commit? src/cpu: * frv.cpu (mabshs): Explicitly sign-extend arguments of abs to DI. (DI-ext-HI, DI-ext-UHI, DI-ext-DI): New pmacros. (media-arith-sat-semantics): Explicitly sign- or zero-extend arguments of "operation" to DI using "mode" and the new pmacros. Index: frv.cpu =================================================================== RCS file: /cvs/src/src/cpu/frv.cpu,v retrieving revision 1.24 diff -u -p -r1.24 frv.cpu --- frv.cpu 5 Jul 2007 09:49:03 -0000 1.24 +++ frv.cpu 23 Dec 2008 21:11:06 -0000 @@ -8229,18 +8229,28 @@ (set FRintk (c-raw-call SI "frv_ref_SI" FRintk)) (set arghi (halfword hi FRintj 0)) (set arglo (halfword lo FRintj 0)) - (saturate-v (abs arghi) 32767 -32768 (msr-sie-fri-hi) + ; We extend the argument before the abs operation so we can + ; notice -32768 overflowing as 32768. + (saturate-v (abs (ext DI arghi)) 32767 -32768 (msr-sie-fri-hi) (halfword hi FRintk 0)) - (saturate-v (abs arglo) 32767 -32768 (msr-sie-fri-lo) + (saturate-v (abs (ext DI arglo)) 32767 -32768 (msr-sie-fri-lo) (halfword lo FRintk 0))) ((fr400 (unit u-media-1)) (fr450 (unit u-media-1)) (fr550 (unit u-media))) ) +; How to extend from a mode to get the intended signedness. +(define-pmacro (DI-ext-HI x) (ext DI x)) +(define-pmacro (DI-ext-UHI x) (zext DI x)) +(define-pmacro (DI-ext-DI x) x) + (define-pmacro (media-arith-sat-semantics operation arg1 arg2 res mode max min sie) (sequence ((DI tmp)) - (set tmp (operation arg1 arg2)) + ; Make sure we saturate at max/min against a value that is + ; sign- or zero-extended appropriately from "mode". + (set tmp (operation DI + ((.sym DI-ext- mode) arg1) ((.sym DI-ext- mode) arg2))) (saturate-v tmp max min sie res)) ) The (for me) more interesting patch, as mentioned, keeps the result of an additive and multiplicative operation, staying in the mode of the operation. I didn't touch logical operators because they don't "overflow" and I also didn't touch div and mod, because they're too loosely defined in corner cases, so they require domain-specific testing of the input operands anyway (consider 0x80..01 / -1 on a x86 host). Committing this patch as-is would expose the longer list of FAILs seen above for frv-elf sim, so I'll wait until the frv issue is resolved and gcc regtests for frv, m32r and sh64 have completed - or of course if someone has a strong argument against it. src/sim/common: * cgen-ops.h (ADDQI, SUBQI, MULQI, NEGQI, ABSQI, ADDHI, SUBHI) (MULHI, NEGHI, ABSHI, ADDSI, SUBSI, MULSI, NEGSI, ABSSI, ADDDI) (SUBDI, MULDI, NEGDI, ABSDI): Cast arguments to the unsigned type variant; UQI, UHI, USI, UDI, and cast the result to the signed type, QI, HI, SI, DI. Index: cgen-ops.h =================================================================== RCS file: /cvs/src/src/sim/common/cgen-ops.h,v retrieving revision 1.9 diff -p -u -r1.9 cgen-ops.h --- cgen-ops.h 1 Jan 2008 22:53:23 -0000 1.9 +++ cgen-ops.h 23 Dec 2008 02:35:47 -0000 @@ -59,9 +59,9 @@ along with this program. If not, see (BI) (y)) #define GEUBI(x, y) ((BI) (x) >= (BI) (y)) -#define ADDQI(x, y) ((x) + (y)) -#define SUBQI(x, y) ((x) - (y)) -#define MULQI(x, y) ((x) * (y)) +#define ADDQI(x, y) ((QI) ((UQI) (x) + (UQI) (y))) +#define SUBQI(x, y) ((QI) ((UQI) (x) - (UQI) (y))) +#define MULQI(x, y) ((QI) ((UQI) (x) * (UQI) (y))) #define DIVQI(x, y) ((QI) (x) / (QI) (y)) #define UDIVQI(x, y) ((UQI) (x) / (UQI) (y)) #define MODQI(x, y) ((QI) (x) % (QI) (y)) @@ -74,10 +74,10 @@ extern QI ROLQI (QI, int); #define ANDQI(x, y) ((x) & (y)) #define ORQI(x, y) ((x) | (y)) #define XORQI(x, y) ((x) ^ (y)) -#define NEGQI(x) (- (x)) +#define NEGQI(x) ((QI) (- (UQI) (x))) #define NOTQI(x) (! (QI) (x)) #define INVQI(x) (~ (x)) -#define ABSQI(x) ((x) < 0 ? -(x) : (x)) +#define ABSQI(x) ((QI) ((QI) (x) < 0 ? -(UQI) (x) : (UQI) (x))) #define EQQI(x, y) ((QI) (x) == (QI) (y)) #define NEQI(x, y) ((QI) (x) != (QI) (y)) #define LTQI(x, y) ((QI) (x) < (QI) (y)) @@ -89,9 +89,9 @@ extern QI ROLQI (QI, int); #define GTUQI(x, y) ((UQI) (x) > (UQI) (y)) #define GEUQI(x, y) ((UQI) (x) >= (UQI) (y)) -#define ADDHI(x, y) ((x) + (y)) -#define SUBHI(x, y) ((x) - (y)) -#define MULHI(x, y) ((x) * (y)) +#define ADDHI(x, y) ((HI) ((UHI) (x) + (UHI) (y))) +#define SUBHI(x, y) ((HI) ((UHI) (x) - (UHI) (y))) +#define MULHI(x, y) ((HI) ((UHI) (x) * (UHI) (y))) #define DIVHI(x, y) ((HI) (x) / (HI) (y)) #define UDIVHI(x, y) ((UHI) (x) / (UHI) (y)) #define MODHI(x, y) ((HI) (x) % (HI) (y)) @@ -104,10 +104,10 @@ extern HI ROLHI (HI, int); #define ANDHI(x, y) ((x) & (y)) #define ORHI(x, y) ((x) | (y)) #define XORHI(x, y) ((x) ^ (y)) -#define NEGHI(x) (- (x)) +#define NEGHI(x) ((HI) (- (UHI) (x))) #define NOTHI(x) (! (HI) (x)) #define INVHI(x) (~ (x)) -#define ABSHI(x) ((x) < 0 ? -(x) : (x)) +#define ABSHI(x) ((HI) ((HI) (x) < 0 ? -(UHI) (x) : (UHI) (x))) #define EQHI(x, y) ((HI) (x) == (HI) (y)) #define NEHI(x, y) ((HI) (x) != (HI) (y)) #define LTHI(x, y) ((HI) (x) < (HI) (y)) @@ -119,9 +119,9 @@ extern HI ROLHI (HI, int); #define GTUHI(x, y) ((UHI) (x) > (UHI) (y)) #define GEUHI(x, y) ((UHI) (x) >= (UHI) (y)) -#define ADDSI(x, y) ((x) + (y)) -#define SUBSI(x, y) ((x) - (y)) -#define MULSI(x, y) ((x) * (y)) +#define ADDSI(x, y) ((SI) ((USI) (x) + (USI) (y))) +#define SUBSI(x, y) ((SI) ((USI) (x) - (USI) (y))) +#define MULSI(x, y) ((SI) ((USI) (x) * (USI) (y))) #define DIVSI(x, y) ((SI) (x) / (SI) (y)) #define UDIVSI(x, y) ((USI) (x) / (USI) (y)) #define MODSI(x, y) ((SI) (x) % (SI) (y)) @@ -134,10 +134,10 @@ extern SI ROLSI (SI, int); #define ANDSI(x, y) ((x) & (y)) #define ORSI(x, y) ((x) | (y)) #define XORSI(x, y) ((x) ^ (y)) -#define NEGSI(x) (- (x)) +#define NEGSI(x) ((SI) (- (USI) (x))) #define NOTSI(x) (! (SI) (x)) #define INVSI(x) (~ (x)) -#define ABSSI(x) ((x) < 0 ? -(x) : (x)) +#define ABSSI(x) ((SI) ((SI) (x) < 0 ? -(USI) (x) : (USI) (x))) #define EQSI(x, y) ((SI) (x) == (SI) (y)) #define NESI(x, y) ((SI) (x) != (SI) (y)) #define LTSI(x, y) ((SI) (x) < (SI) (y)) @@ -179,9 +179,9 @@ extern int LEUDI (UDI, UDI); extern int GTUDI (UDI, UDI); extern int GEUDI (UDI, UDI); #else /* ! DI_FN_SUPPORT */ -#define ADDDI(x, y) ((x) + (y)) -#define SUBDI(x, y) ((x) - (y)) -#define MULDI(x, y) ((x) * (y)) +#define ADDDI(x, y) ((DI) ((UDI) (x) + (UDI) (y))) +#define SUBDI(x, y) ((DI) ((UDI) (x) - (UDI) (y))) +#define MULDI(x, y) ((DI) ((UDI) (x) * (UDI) (y))) #define DIVDI(x, y) ((DI) (x) / (DI) (y)) #define UDIVDI(x, y) ((UDI) (x) / (UDI) (y)) #define MODDI(x, y) ((DI) (x) % (DI) (y)) @@ -194,10 +194,10 @@ extern DI ROLDI (DI, int); #define ANDDI(x, y) ((x) & (y)) #define ORDI(x, y) ((x) | (y)) #define XORDI(x, y) ((x) ^ (y)) -#define NEGDI(x) (- (x)) +#define NEGDI(x) ((DI) (- (UDI) (x))) #define NOTDI(x) (! (DI) (x)) #define INVDI(x) (~ (x)) -#define ABSDI(x) ((x) < 0 ? -(x) : (x)) +#define ABSDI(x) ((DI) ((DI) (x) < 0 ? -(UDI) (x) : (UDI) (x))) #define EQDI(x, y) ((DI) (x) == (DI) (y)) #define NEDI(x, y) ((DI) (x) != (DI) (y)) #define LTDI(x, y) ((DI) (x) < (DI) (y)) Happy Holidays and seasonal greetings. brgds, H-P