Re: [PATCH] Optimize store_expr from STRING_CST [PR95052]
On Thu, 2020-05-14 at 18:51 +0200, Jakub Jelinek wrote: > On Thu, May 14, 2020 at 10:10:55AM -0600, Jeff Law wrote: > > On Tue, 2020-05-12 at 10:12 +0200, Jakub Jelinek wrote: > > > Hi! > > > > > > In the following testcase, store_expr of e.g. 97 bytes long string literal > > > into 1MB long array is implemented by copying the 97 bytes from .rodata > > > section, followed by clearing the remaining bytes. But, as the STRING_CST > > > has type char[1024*1024], we actually allocate whole 1MB in .rodata > > > section > > > for it, even when we only use the first 97 bytes from that. > > > > > > The following patch tweaks it so that if we are going to initialize only > > > the > > > small part from it, we don't emit all the zeros that we never use after > > > it. > > > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > > > 2020-05-12 Jakub Jelinek > > > > > > PR middle-end/95052 > > > * expr.c (store_expr): If expr_size is constant and significantly > > > larger than TREE_STRING_LENGTH, set temp to just the > > > TREE_STRING_LENGTH portion of the STRING_CST. > > > > > > * gcc.target/i386/pr95052.c: New test. > > OK. Initially I had a number of concerns, but this only affects the .rodata > > that's used to initialize the real object. The .rodata bits have no other > > purpose and aren't otherwise accessable. > > I wasn't sure if it wouldn't be safer to add some bool flag set to true by > the new code and then add gcc_assert in all the other paths, like following > incremental patch. I believe none of the asserts can trigger right now, > but the code is still adjusting what it plans to use as source before actually > only copying fewer bytes from it, so if somebody changes it later... > > Thoughts on that? Can't hurt, and debugging the assert tripping is likely a hell of a lot easier than debugging the resultant incorrect code. So if it passes, then I'd say go for it. jeff >
Re: [PATCH] Optimize store_expr from STRING_CST [PR95052]
On Thu, May 14, 2020 at 10:10:55AM -0600, Jeff Law wrote: > On Tue, 2020-05-12 at 10:12 +0200, Jakub Jelinek wrote: > > Hi! > > > > In the following testcase, store_expr of e.g. 97 bytes long string literal > > into 1MB long array is implemented by copying the 97 bytes from .rodata > > section, followed by clearing the remaining bytes. But, as the STRING_CST > > has type char[1024*1024], we actually allocate whole 1MB in .rodata section > > for it, even when we only use the first 97 bytes from that. > > > > The following patch tweaks it so that if we are going to initialize only the > > small part from it, we don't emit all the zeros that we never use after it. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > 2020-05-12 Jakub Jelinek > > > > PR middle-end/95052 > > * expr.c (store_expr): If expr_size is constant and significantly > > larger than TREE_STRING_LENGTH, set temp to just the > > TREE_STRING_LENGTH portion of the STRING_CST. > > > > * gcc.target/i386/pr95052.c: New test. > OK. Initially I had a number of concerns, but this only affects the .rodata > that's used to initialize the real object. The .rodata bits have no other > purpose and aren't otherwise accessable. I wasn't sure if it wouldn't be safer to add some bool flag set to true by the new code and then add gcc_assert in all the other paths, like following incremental patch. I believe none of the asserts can trigger right now, but the code is still adjusting what it plans to use as source before actually only copying fewer bytes from it, so if somebody changes it later... Thoughts on that? --- gcc/expr.c.jj 2020-05-14 18:43:46.324145031 +0200 +++ gcc/expr.c 2020-05-14 18:47:19.003950500 +0200 @@ -5583,6 +5583,7 @@ store_expr (tree exp, rtx target, int ca rtx temp; rtx alt_rtl = NULL_RTX; location_t loc = curr_insn_location (); + bool shortened_string_cst = false; if (VOID_TYPE_P (TREE_TYPE (exp))) { @@ -5771,6 +5772,7 @@ store_expr (tree exp, rtx target, int ca = build_index_type (size_int (TREE_STRING_LENGTH (exp) - 1)); TREE_TYPE (rexp) = build_array_type (TREE_TYPE (TREE_TYPE (exp)), index); + shortened_string_cst = true; } } temp = expand_expr_real (rexp, tmp_target, GET_MODE (target), @@ -5787,6 +5789,7 @@ store_expr (tree exp, rtx target, int ca && TREE_CODE (exp) != ERROR_MARK && GET_MODE (target) != TYPE_MODE (TREE_TYPE (exp))) { + gcc_assert (!shortened_string_cst); if (GET_MODE_CLASS (GET_MODE (target)) != GET_MODE_CLASS (TYPE_MODE (TREE_TYPE (exp))) && known_eq (GET_MODE_BITSIZE (GET_MODE (target)), @@ -5839,6 +5842,7 @@ store_expr (tree exp, rtx target, int ca { if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != VOIDmode) { + gcc_assert (!shortened_string_cst); if (GET_MODE (target) == BLKmode) { /* Handle calls that return BLKmode values in registers. */ @@ -5924,6 +5928,8 @@ store_expr (tree exp, rtx target, int ca emit_label (label); } } + else if (shortened_string_cst) + gcc_unreachable (); /* Handle calls that return values in multiple non-contiguous locations. The Irix 6 ABI has examples of this. */ else if (GET_CODE (target) == PARALLEL) @@ -5953,6 +5959,8 @@ store_expr (tree exp, rtx target, int ca emit_move_insn (target, temp); } } + else +gcc_assert (!shortened_string_cst); return NULL_RTX; } Jakub
Re: [PATCH] Optimize store_expr from STRING_CST [PR95052]
On Tue, 2020-05-12 at 10:12 +0200, Jakub Jelinek wrote: > Hi! > > In the following testcase, store_expr of e.g. 97 bytes long string literal > into 1MB long array is implemented by copying the 97 bytes from .rodata > section, followed by clearing the remaining bytes. But, as the STRING_CST > has type char[1024*1024], we actually allocate whole 1MB in .rodata section > for it, even when we only use the first 97 bytes from that. > > The following patch tweaks it so that if we are going to initialize only the > small part from it, we don't emit all the zeros that we never use after it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2020-05-12 Jakub Jelinek > > PR middle-end/95052 > * expr.c (store_expr): If expr_size is constant and significantly > larger than TREE_STRING_LENGTH, set temp to just the > TREE_STRING_LENGTH portion of the STRING_CST. > > * gcc.target/i386/pr95052.c: New test. OK. Initially I had a number of concerns, but this only affects the .rodata that's used to initialize the real object. The .rodata bits have no other purpose and aren't otherwise accessable. jeff >
[PATCH] Optimize store_expr from STRING_CST [PR95052]
Hi! In the following testcase, store_expr of e.g. 97 bytes long string literal into 1MB long array is implemented by copying the 97 bytes from .rodata section, followed by clearing the remaining bytes. But, as the STRING_CST has type char[1024*1024], we actually allocate whole 1MB in .rodata section for it, even when we only use the first 97 bytes from that. The following patch tweaks it so that if we are going to initialize only the small part from it, we don't emit all the zeros that we never use after it. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2020-05-12 Jakub Jelinek PR middle-end/95052 * expr.c (store_expr): If expr_size is constant and significantly larger than TREE_STRING_LENGTH, set temp to just the TREE_STRING_LENGTH portion of the STRING_CST. * gcc.target/i386/pr95052.c: New test. --- gcc/expr.c.jj 2020-04-16 12:57:16.584912448 +0200 +++ gcc/expr.c 2020-05-11 20:58:46.436785403 +0200 @@ -5749,7 +5749,31 @@ store_expr (tree exp, rtx target, int ca /* If we want to use a nontemporal or a reverse order store, force the value into a register first. */ tmp_target = nontemporal || reverse ? NULL_RTX : target; - temp = expand_expr_real (exp, tmp_target, GET_MODE (target), + tree rexp = exp; + if (TREE_CODE (exp) == STRING_CST + && tmp_target == target + && GET_MODE (target) == BLKmode + && TYPE_MODE (TREE_TYPE (exp)) == BLKmode) + { + rtx size = expr_size (exp); + if (CONST_INT_P (size) + && size != const0_rtx + && (UINTVAL (size) + > ((unsigned HOST_WIDE_INT) TREE_STRING_LENGTH (exp) + 32))) + { + /* If the STRING_CST has much larger array type than +TREE_STRING_LENGTH, only emit the TREE_STRING_LENGTH part of +it into the rodata section as the code later on will use +memset zero for the remainder anyway. See PR95052. */ + tmp_target = NULL_RTX; + rexp = copy_node (exp); + tree index + = build_index_type (size_int (TREE_STRING_LENGTH (exp) - 1)); + TREE_TYPE (rexp) = build_array_type (TREE_TYPE (TREE_TYPE (exp)), + index); + } + } + temp = expand_expr_real (rexp, tmp_target, GET_MODE (target), (call_param_p ? EXPAND_STACK_PARM : EXPAND_NORMAL), &alt_rtl, false); --- gcc/testsuite/gcc.target/i386/pr95052.c.jj 2020-05-11 21:03:05.635238485 +0200 +++ gcc/testsuite/gcc.target/i386/pr95052.c 2020-05-11 21:02:47.473487020 +0200 @@ -0,0 +1,20 @@ +/* PR middle-end/95052 */ +/* { dg-do compile } */ +/* { dg-options "-Os -mtune=skylake" } */ +/* Verify we don't waste almost 2 megabytes of .rodata. */ +/* { dg-final { scan-assembler-not "\.zero\t1048\[0-9]\[0-9]\[0-9]" } } */ +extern void foo (char *, unsigned); + +int +main () +{ + char str[1024 * 1024] = + "fooiuhluhpiuhliuhliyfyukyfklyugkiuhpoipoipoipoipoipoipoipoipoipoipoipoipoimipoipiuhoulouihnliuhl"; + char arr[1024 * 1024] = +{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 6, 2, 3, + 4, 5, 6, 7, 8, 9, 0, 3, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, + 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0 }; + foo (str, sizeof (str)); + foo (arr, sizeof (arr)); + return 0; +} Jakub