[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Jakub Jelinek changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #19 from Jakub Jelinek 2012-01-26 14:11:24 UTC --- Fixed.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #18 from Jakub Jelinek 2012-01-26 14:09:34 UTC --- Author: jakub Date: Thu Jan 26 14:09:29 2012 New Revision: 183560 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183560 Log: PR middle-end/51895 * expr.c (expand_expr_real_1): Handle BLKmode MEM_REF of non-addressable non-BLKmode base correctly. * g++.dg/opt/pr51895.C: New test. Added: trunk/gcc/testsuite/g++.dg/opt/pr51895.C Modified: trunk/gcc/ChangeLog trunk/gcc/expr.c trunk/gcc/testsuite/ChangeLog
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #17 from Richard Guenther 2012-01-23 11:59:57 UTC --- Author: rguenth Date: Mon Jan 23 11:59:53 2012 New Revision: 183429 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183429 Log: 2012-01-23 Richard Guenther PR tree-optimization/51895 * tree-sra.c (decide_one_param_reduction): Avoid sub-optimal parameter decomposition into BLKmode components. Modified: trunk/gcc/ChangeLog trunk/gcc/tree-sra.c
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #16 from Jakub Jelinek 2012-01-20 16:49:52 UTC --- Created attachment 26396 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26396 l.ii Note that the BLKmode MEM_REF with non-BLKmode base expansion can be triggered also without SRA, e.g. this (distilled from locale-inst.cc) reaches the code on i?86-linux with -m32 -O2 -fno-ipa-sra -fno-tree-sra.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895
--- Comment #15 from Richard Guenther 2012-01-20
14:40:09 UTC ---
Testing
@@ -3891,6 +3853,8 @@ decide_one_param_reduction (struct acces
{
by_ref = false;
agg_size = cur_parm_size;
+ if (DECL_MODE (parm) != BLKmode)
+return 0;
}
if (dump_file)
as passing a parameter by value in non-BLKmode (on the PARM_DECL) should
be good evidence to not split it down further (in this case we have
TYPE_SIZE != MODE_SIZE as well, but that's another story).
Hopefully DECL_MODE on the PARM_DECL is good enough and reflects actual
argument passing closely enough ... ?
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Richard Guenther changed: What|Removed |Added Target|powerpc64-linux |powerpc64-linux, i?86-*-* --- Comment #14 from Richard Guenther 2012-01-20 14:20:17 UTC --- Same ICE on i?86-linux btw.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Richard Guenther changed: What|Removed |Added CC||rguenth at gcc dot gnu.org --- Comment #13 from Richard Guenther 2012-01-20 14:18:45 UTC --- I'll look at the IPA-SRA issue.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #12 from Eric Botcazou 2012-01-20 09:41:41 UTC --- > Sure, but I believe just that eipa_sra is what is causing this pessimization > by > doing a bad decision. Precisely, so why not change that? > Anyway, if you prefer one of the other 3 patches (for now?), which one it is? Yours looks less hackish for sure. :-)
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #11 from Jakub Jelinek 2012-01-20 09:07:54 UTC --- (In reply to comment #10) > > I wonder why it does this, instead of just using type S, and if it really > > has > > to for some reason, why it can't at least make sure it has the same > > TYPE_MODE. > > Changing a TImode argument to a BLKmode argument doesn't look at least like > > a > > good optimization. > > I concur, BLKmode means spilling to memory at some point, so this looks like a > clear pessimization to me. Sure, but I believe just that eipa_sra is what is causing this pessimization by doing a bad decision. Anyway, if you prefer one of the other 3 patches (for now?), which one it is? Or some other place to handle it?
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #10 from Eric Botcazou 2012-01-19 16:13:55 UTC --- > I wonder why it does this, instead of just using type S, and if it really has > to for some reason, why it can't at least make sure it has the same TYPE_MODE. > Changing a TImode argument to a BLKmode argument doesn't look at least like a > good optimization. I concur, BLKmode means spilling to memory at some point, so this looks like a clear pessimization to me.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #9 from Jakub Jelinek 2012-01-19 13:55:31 UTC --- Peter's patch does as well. The thing is that we still do wrong expand_expr on that kind of MEM_EXPR, and I don't see anything that would prevent such MEM_EXPRs in all kinds of other contexts, not just in the loading of the parameters. And each of the would need hacks to cope with a BLKmode expression surprisingly being expanded to something that doesn't have BLKmode.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895
--- Comment #8 from Richard Guenther 2012-01-19
13:17:25 UTC ---
For example (untested, and probably completely bogus):
Index: expr.c
===
--- expr.c (revision 183296)
+++ expr.c (working copy)
@@ -1517,7 +1517,9 @@ move_block_to_reg (int regno, rtx x, int
for (i = 0; i < nregs; i++)
emit_move_insn (gen_rtx_REG (word_mode, regno + i),
- operand_subword_force (x, i, mode));
+ operand_subword_force (x, i,
+ CONSTANT_P (x)
+ ? mode : GET_MODE (x)));
}
/* Copy all or part of a BLKmode value X out of registers starting at REGNO.
As written elsewhere operand_subword* should be deprecated ... so maybe
we can use sth else in move_block_to_reg?
The same effect could be achieved by changing operand_subword to adhere
to its documentation and ignore mode if op is !CONST_INT_P ... thus
Index: emit-rtl.c
===
--- emit-rtl.c (revision 183296)
+++ emit-rtl.c (working copy)
@@ -1367,7 +1367,7 @@ paradoxical_subreg_p (const_rtx x)
rtx
operand_subword (rtx op, unsigned int offset, int validate_address, enum
machine_mode mode)
{
- if (mode == VOIDmode)
+ if (!CONST_INT_P (op))
mode = GET_MODE (op);
gcc_assert (mode != VOIDmode);
Both fix the testcase.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Jakub Jelinek changed: What|Removed |Added CC||ebotcazou at gcc dot ||gnu.org --- Comment #7 from Jakub Jelinek 2012-01-19 13:11:13 UTC --- (In reply to comment #6) > > Untested patch that attempts to fix BLKmode MEM_REF expansion with > > non-DECL_ADDRESSABLE non-BLKmode base. It creates abysmal code, so IMNSHO > > eipa_sra should be fixed not to do this. > > Hm, can't we do better using extract_bit_field? I mean, it definitely > should work to do any BIT_FIELD_REF on an rvalue, even if it is a register. > The patch from comment #1 doesn't look completely wrong, it just seems that > the caller should have catered for using the mode of the reg. The docs > of operand_subword also say 'MODE is the mode of OP in case it is a CONST_INT' > so MODE should be irrelevant if REG_P (op) ... > > Seems to be a tricky area, but using a stack temporary looks like overkill. We don't have BLKmode pseudos, the only thing that can be BLKmode is MEM. So I'm afraid we can't avoid that. E.g. VIEW_CONVERT_EXPR of a TImode value to BLKmode value would be expanded by spilling the TImode value to a stack temporary and adjust_address it to BLKmode too.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Richard Guenther changed: What|Removed |Added Priority|P3 |P1 --- Comment #6 from Richard Guenther 2012-01-19 13:03:14 UTC --- (In reply to comment #5) > Created attachment 26377 [details] > gcc47-pr51895.patch > > Untested patch that attempts to fix BLKmode MEM_REF expansion with > non-DECL_ADDRESSABLE non-BLKmode base. It creates abysmal code, so IMNSHO > eipa_sra should be fixed not to do this. Hm, can't we do better using extract_bit_field? I mean, it definitely should work to do any BIT_FIELD_REF on an rvalue, even if it is a register. The patch from comment #1 doesn't look completely wrong, it just seems that the caller should have catered for using the mode of the reg. The docs of operand_subword also say 'MODE is the mode of OP in case it is a CONST_INT' so MODE should be irrelevant if REG_P (op) ... Seems to be a tricky area, but using a stack temporary looks like overkill.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #5 from Jakub Jelinek 2012-01-19 12:33:55 UTC --- Created attachment 26377 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26377 gcc47-pr51895.patch Untested patch that attempts to fix BLKmode MEM_REF expansion with non-DECL_ADDRESSABLE non-BLKmode base. It creates abysmal code, so IMNSHO eipa_sra should be fixed not to do this.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #4 from Jakub Jelinek 2012-01-19 10:16:07 UTC --- (In reply to comment #3) > > bftype = TREE_TYPE (base); > > if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode) > > bftype = TREE_TYPE (exp); > > return expand_expr (build3 (BIT_FIELD_REF, bftype, > > base, > > TYPE_SIZE (TREE_TYPE (exp)), > > bit_offset), > > target, tmode, modifier); > > base here is TImode (x PARM_DECL), but exp is BLKmode, so this returns a > > TImode > > pseudo. Shouldn't it store it into a BLKmode temporary and return that MEM > > instead? > > Using a BIT_FIELD_REF looked most convenient. Using extract_bit_field > may also be an option (which I suppose is what the above ends up doing?) I think if exp is BLKmode, then we don't want to do a BIT_FIELD_REF nor extract_bit_field. We IMHO need to store base into a temporary and just adjust the MEM. Or extract the bit field and then store it into a temporary and adjust, but the former looks easier.
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Richard Guenther changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2012-01-19 Target Milestone|--- |4.7.0 Ever Confirmed|0 |1 --- Comment #3 from Richard Guenther 2012-01-19 10:06:38 UTC --- (In reply to comment #2) > This starts with eipa_sra. It changes a S argument (which has TImode > TYPE_MODE) into char [9] (with BLKmode)) and then on both caller and callee > side we have on one side a BLKmode type and on the other side a BLKmode > MEM_REF > with pointer to TImode on the second MEM_REF operand. > I wonder why it does this, instead of just using type S, and if it really has > to for some reason, why it can't at least make sure it has the same TYPE_MODE. > Changing a TImode argument to a BLKmode argument doesn't look at least like a > good optimization. > > Or the bug is in the MEM_REF expansion, which expands a BLKmode MEM_REF into a > TImode reg: > bftype = TREE_TYPE (base); > if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode) > bftype = TREE_TYPE (exp); > return expand_expr (build3 (BIT_FIELD_REF, bftype, > base, > TYPE_SIZE (TREE_TYPE (exp)), > bit_offset), > target, tmode, modifier); > base here is TImode (x PARM_DECL), but exp is BLKmode, so this returns a > TImode > pseudo. Shouldn't it store it into a BLKmode temporary and return that MEM > instead? Using a BIT_FIELD_REF looked most convenient. Using extract_bit_field may also be an option (which I suppose is what the above ends up doing?)
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 Jakub Jelinek changed: What|Removed |Added CC||jamborm at gcc dot gnu.org --- Comment #2 from Jakub Jelinek 2012-01-19 07:33:07 UTC --- This starts with eipa_sra. It changes a S argument (which has TImode TYPE_MODE) into char [9] (with BLKmode)) and then on both caller and callee side we have on one side a BLKmode type and on the other side a BLKmode MEM_REF with pointer to TImode on the second MEM_REF operand. I wonder why it does this, instead of just using type S, and if it really has to for some reason, why it can't at least make sure it has the same TYPE_MODE. Changing a TImode argument to a BLKmode argument doesn't look at least like a good optimization. Or the bug is in the MEM_REF expansion, which expands a BLKmode MEM_REF into a TImode reg: bftype = TREE_TYPE (base); if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode) bftype = TREE_TYPE (exp); return expand_expr (build3 (BIT_FIELD_REF, bftype, base, TYPE_SIZE (TREE_TYPE (exp)), bit_offset), target, tmode, modifier); base here is TImode (x PARM_DECL), but exp is BLKmode, so this returns a TImode pseudo. Shouldn't it store it into a BLKmode temporary and return that MEM instead?
[Bug middle-end/51895] [4.7 Regression] ICE in simplify_subreg
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51895 --- Comment #1 from Peter Bergner 2012-01-19 02:42:44 UTC --- We ICE when we try and do a BLK move from a reg:TI. In GCC 4.6, we don't see this, since the args[i].tree_value is different: (gdb-gcc4.6) ptree args[i].tree_value unit size align 64 symtab 0 alias set 43 canonical type 0xfffafd6f960 fields used private unsigned nonlocal decl_3 DI file t.ii line 30433 col 11 size unit size align 64 offset_align 128 offset bit offset context chain > context full-name "class llvm::SDValue" needs-constructor X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown pointer_to_this reference_to_this chain > used TI file t.ii line 45352 col 9 size unit size align 64 context abstract_origin (reg/v:TI 306 [ Ptr1 ])> versus for gcc 4.7: (gdb-gc4.7) ptree args[i].tree_value unit size align 8 symtab 0 alias set -1 canonical type 0xfffb5e403f0 precision 8 min max pointer_to_this reference_to_this > BLK size unit size align 8 symtab 0 alias set 0 canonical type 0xfffb0cac198 domain DI size unit size align 64 symtab 0 alias set -1 canonical type 0xfffb0cac0f0 precision 64 min max > pointer_to_this > arg 0 sizes-gimplified public unsigned type_6 DI size unit size align 64 symtab 0 alias set 2 canonical type 0xfffa8d50b28 pointer_to_this > arg 0 used TI file t.ii line 45286 col 6 size unit size align 64 context abstract_origin (reg/v:TI 279 [ Ptr1 ])>> arg 1 constant 0>> That difference forces us down different paths starting at this load_register_parameters() test: else if (TYPE_MODE (TREE_TYPE (args[i].tree_value)) == BLKmode) For 4.6, the mode is a TImode and for 4.7, it's a BLKmode. The following patch seems to fix it: Index: gcc/emit-rtl.c === --- gcc/emit-rtl.c (revision 183280) +++ gcc/emit-rtl.c (working copy) @@ -1401,6 +1401,9 @@ operand_subword (rtx op, unsigned int of return replace_equiv_address (new_rtx, XEXP (new_rtx, 0)); } + if (REG_P (op) && mode == BLKmode) +mode = GET_MODE (op); + /* Rest can be handled by simplify_subreg. */ return simplify_gen_subreg (word_mode, op, mode, (offset * UNITS_PER_WORD)); } I'm bootstraping/regtesting this patch now.
