The testcase for arm is: #include <stdio.h> unsigned int b; int c; signed char fn1() { signed char d; for (int i = 0; i < 1; i++) d = -15; return d; } int main() { for (c = 0; c < 1; c++) b = 0; char e = fn1(); signed char f = e ^ b; printf("checksum = %x\n", (int)f); } As of r226139 I'm seeing this printing: checksum = f1 whereas before it printed: checksum = fffffff1 This is at -O1. Curiously -O2 and other optimisations print the expected: checksum = fffffff1 There is a zero-extension sneaking in where it didn't before. If I look at the tree dumps before and after r226139 I see the difference starts at forwprop4 but it doesn't seem to be wrong to me. Before: signed charD.14 fD.5068; charD.4 eD.5067; signed charD.14 _8; intD.3 _14; ;; basic block 2, loop depth 0, count 0, freq 900, maybe hot ;; prev block 0, next block 1, flags: (NEW, REACHABLE) ;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE) # .MEM_23 = VDEF <.MEM_2(D)> bD.5053 = 0; # .MEM_25 = VDEF <.MEM_23> cD.5054 = 1; _8 = fn1D.5055 (); e_9 = (charD.4) _8; f_13 = _8; _14 = (intD.3) f_13; # .MEM_15 = VDEF <.MEM_25> # USE = nonlocal null # CLB = nonlocal null printfD.782 ("checksum = %x\n", _14); # VUSE <.MEM_15> return 0; and after: signed charD.14 fD.5068; charD.4 eD.5067; signed charD.14 _8; intD.3 _14; ;; basic block 2, loop depth 0, count 0, freq 900, maybe hot ;; prev block 0, next block 1, flags: (NEW, REACHABLE) ;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE) # .MEM_23 = VDEF <.MEM_2(D)> bD.5053 = 0; # .MEM_25 = VDEF <.MEM_23> cD.5054 = 1; _8 = fn1D.5055 (); e_9 = (charD.4) _8; f_13 = _8; _14 = (intD.3) _8; <<<==== This was _14 = (intD.3) f_13 before. # .MEM_15 = VDEF <.MEM_25> # USE = nonlocal null # CLB = nonlocal null printfD.782 ("checksum = %x\n", _14); # VUSE <.MEM_15> return 0; ;; succ: EXIT [100.0%] The zero-extension sneaks in during expand: ;; f_13 = _8; (insn 15 14 0 (set (reg/v:SI 110 [ f ]) (zero_extend:SI (subreg/u:QI (reg/v:SI 110 [ f ]) 0))) arm-zext.c:18 -1 (nil)) Looking at the expansion of that in gdb I see in expand_assignment that 'to' and 'from' seem to have correct signed types: (gdb) call debug_tree (to) <ssa_name 0x7ffff7238750 type <integer_type 0x7ffff7321348 signed char sizes-gimplified public string-flag QI size <integer_cst 0x7ffff732e000 constant 8> unit size <integer_cst 0x7ffff732e018 constant 1> align 8 symtab 0 alias set -1 canonical type 0x7ffff7321348 precision 8 min <integer_cst 0x7ffff731cfa8 -128> max <integer_cst 0x7ffff731cfd8 127> context <translation_unit_decl 0x7ffff7226d20 D.5069>> var <var_decl 0x7ffff7247090 f>def_stmt f_13 = _8; version 13> (gdb) call debug_tree (from) <ssa_name 0x7ffff72385e8 type <integer_type 0x7ffff7321348 signed char sizes-gimplified public string-flag QI size <integer_cst 0x7ffff732e000 constant 8> unit size <integer_cst 0x7ffff732e018 constant 1> align 8 symtab 0 alias set -1 canonical type 0x7ffff7321348 precision 8 min <integer_cst 0x7ffff731cfa8 -128> max <integer_cst 0x7ffff731cfd8 127> context <translation_unit_decl 0x7ffff7226d20 D.5069>> visiteddef_stmt _8 = fn1 (); version 8> The problem seems to occur one level below in store_expr_with_bounds where exp is changed to have an unsigned type and in particular this code that changes the type based on SUBREG_PROMOTED_SIGN: if (!SUBREG_CHECK_PROMOTED_SIGN (target, TYPE_UNSIGNED (TREE_TYPE (exp)))) { /* Some types, e.g. Fortran's logical*4, won't have a signed version, so use the mode instead. */ tree ntype = (signed_or_unsigned_type_for (SUBREG_PROMOTED_SIGN (target), TREE_TYPE (exp))); if (ntype == NULL) ntype = lang_hooks.types.type_for_mode (TYPE_MODE (TREE_TYPE (exp)), SUBREG_PROMOTED_SIGN (target)); exp = fold_convert_loc (loc, ntype, exp); } The type of exp as it enters store_expr_with_bounds is: <integer_type 0x7f7e44528348 signed char sizes-gimplified public string-flag QI size <integer_cst 0x7f7e44535000 type <integer_type 0x7f7e44528150 bitsizetype> constant 8> unit size <integer_cst 0x7f7e44535018 type <integer_type 0x7f7e445280a8 sizetype> constant 1> align 8 symtab 0 alias set -1 canonical type 0x7f7e44528348 precision 8 min <integer_cst 0x7f7e44523fa8 -128> max <integer_cst 0x7f7e44523fd8 127> context <translation_unit_decl 0x7f7e4442dd20 D.5069>> whereas after this snippet it becomes: <integer_type 0x7f7e4444f0a8 public unsigned QI size <integer_cst 0x7f7e44535000 type <integer_type 0x7f7e44528150 bitsizetype> constant 8> unit size <integer_cst 0x7f7e44535018 type <integer_type 0x7f7e445280a8 sizetype> constant 1> align 8 symtab 0 alias set -1 canonical type 0x7f7e4444f0a8 precision 8 min <integer_cst 0x7f7e4444d120 0> max <integer_cst 0x7f7e443deee8 255>> Kugan, I believe you introduced the SUBREG_PROMOTED_SIGN machinery. Do you think r226139 just exposes a pre-existing bug in that functionality?
.... _8 = fn1D.5055 (); e_9 = (charD.4) _8; f_13 = _8; ... Looks like it is the same issue I saw in https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00403.html Let me look into it.
It is the same issue and the patch posted in https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00403.html fixes this test-case too.
(In reply to kugan from comment #2) > It is the same issue and the patch posted in > https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00403.html fixes this > test-case too. Thanks for checking! I suppose you can reference this PR in that patches ChangeLog when it goes in
Confirmed.
Changing to P1 as it is a wrong-code regression affecting arm
Changing back to P3 as it's not my call to make on second thought...
Back to P1 and classify as target.
BTW, I think your initial prioritization to P1 was fine Kyrylo. In general I strongly support regular developers making this kind of call -- and the worst that happens is there's disagreement about the priority and we can always hash that out on the list.
Created attachment 37543 [details] Use SUBREG_PROMOTED_SIGNED_P in place of SUBREG_PROMOTES_SIGN Hi Guys, I am currently testing this patch locally. It appears to fix the bug, and I hope that it will not introduce any regressions. We shall see. (I am testing arm-eabi and x86_64-linux-gnu). The problem, it appears to me is that the code in store_expr_with_bounds() is using the SUBREG_PROMPTED_SIGN macro, which returns a tri-state value, as a parameter to signed_or_unsigned_type_for(). This function, I think, but I am not 100% sure, uses the parameter as a boolean, so really it should just be told if the promotion is signed or unsigned, nothing else. Cheers Nick
Hi Nick, For this failure (among others) I proposed the series at: https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01713.html that changes the PROMOTE_MODE implementation on arm to be consistent with the TARGET_PROMOTE_FUNCTION_MODE implementation i.e. to not do unsigned promotion of short types
Hi Kyrill, > For this failure (among others) I proposed the series at: > https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01713.html Ah - much better! I have approved the parts of your series that I can, In fact you should have enough approval now to check in everything except the new test. Cheers Nick PS. For the record my patch had one regression: c-c++-common/torture/pr61184.c when compiled with -mthumb and -Os.
(In reply to Nick Clifton from comment #11) > Hi Kyrill, > > > For this failure (among others) I proposed the series at: > > https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01713.html > > Ah - much better! I have approved the parts of your series that I can, In > fact you should have enough approval now to check in everything except the > new test. > > Cheers > Nick > > PS. For the record my patch had one regression: > c-c++-common/torture/pr61184.c when compiled with -mthumb and -Os. Thanks! As I mentioned in the cover letter for the series it is based on a patch by Jim Wilson posted at: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02132.html So I'd like approval for the arm.h hunk of that as well before committing the series if possible
Author: ktkachov Date: Thu Feb 4 09:50:12 2016 New Revision: 233130 URL: https://gcc.gnu.org/viewcvs?rev=233130&root=gcc&view=rev Log: [ARM] PR target/65932: stop changing signedness in PROMOTE_MODE 2016-02-04 Jim Wilson <jim.wilson@linaro.org> PR target/65932 PR target/67714 * config/arm/arm.h (PROMOTE_MODE): Don't set UNSIGNEDP for QImode and HImode. Modified: trunk/gcc/ChangeLog trunk/gcc/config/arm/arm.h
Author: ktkachov Date: Thu Feb 4 09:51:35 2016 New Revision: 233131 URL: https://gcc.gnu.org/viewcvs?rev=233131&root=gcc&view=rev Log: [ARM][1/4] PR target/65932: Add testcase PR target/65932 PR target/67714 * gcc.c-torture/execute/pr67714.c: New test. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr67714.c Modified: trunk/gcc/testsuite/ChangeLog
Author: ktkachov Date: Thu Feb 4 09:54:37 2016 New Revision: 233132 URL: https://gcc.gnu.org/viewcvs?rev=233132&root=gcc&view=rev Log: [ARM][2/4] Fix operand costing logic for SMUL[TB][TB] PR target/65932 PR target/67714 * config/arm/arm.c (arm_new_rtx_costs, MULT case): Properly extract the operands of the SIGN_EXTENDs from a SMUL[TB][TB] rtx. Modified: trunk/gcc/ChangeLog trunk/gcc/config/arm/arm.c
Author: ktkachov Date: Thu Feb 4 09:56:13 2016 New Revision: 233133 URL: https://gcc.gnu.org/viewcvs?rev=233133&root=gcc&view=rev Log: [cse][3/4] Don't overwrite original rtx when folding source of set PR target/65932 PR target/67714 * cse.c (cse_insn): Pass NULL to fold_rtx when initially folding the source of a SET. Modified: trunk/gcc/ChangeLog trunk/gcc/cse.c
Author: ktkachov Date: Thu Feb 4 09:57:36 2016 New Revision: 233134 URL: https://gcc.gnu.org/viewcvs?rev=233134&root=gcc&view=rev Log: [ARM][4/4] Adjust gcc.target/arm/wmul-[123].c tests PR target/65932 PR target/67714 * gcc.target/arm/wmul-3.c: Simplify test to generate just a single smulbb instruction. * gcc.target/amr/wmul-1.c: Add -mtune=cortex-a9 to dg-options. * gcc.target/amr/wmul-2.c: Likewise. Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.target/arm/wmul-1.c trunk/gcc/testsuite/gcc.target/arm/wmul-2.c trunk/gcc/testsuite/gcc.target/arm/wmul-3.c
Fixed on trunk.
Author: ktkachov Date: Tue Feb 16 15:11:26 2016 New Revision: 233454 URL: https://gcc.gnu.org/viewcvs?rev=233454&root=gcc&view=rev Log: [ARM] PR target/65932: stop changing signedness in PROMOTE_MODE Backport from trunk: 2016-02-04 Jim Wilson <jim.wilson@linaro.org> PR target/65932 PR target/67714 * config/arm/arm.h (PROMOTE_MODE): Don't set UNSIGNEDP for QImode and HImode. Modified: branches/gcc-5-branch/gcc/ChangeLog branches/gcc-5-branch/gcc/config/arm/arm.h
Author: clyon Date: Mon Mar 7 08:18:25 2016 New Revision: 234019 URL: https://gcc.gnu.org/viewcvs?rev=234019&root=gcc&view=rev Log: 2016-03-07 Christophe Lyon <christophe.lyon@linaro.org> Backport from mainline 2016-02-04 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/65932 PR target/67714 * config/arm/arm.c (arm_new_rtx_costs, MULT case): Properly extract the operands of the SIGN_EXTENDs from a SMUL[TB][TB] rtx. Modified: branches/gcc-5-branch/gcc/ChangeLog branches/gcc-5-branch/gcc/config/arm/arm.c