Re: [PATCH 2/3] Move MEM_REF expansion to a new function
Hi, On Mon, 12 Mar 2012, Martin Jambor wrote: OK, the following patch changes the places where I previously called the new function to call expand_expr with EXPAND_WRITE modifier and then makes sure we do not perform reads into rvalues in expand_expr_real_1 in the contexts where I need to avoid that. So far it has passed bootstrap and testing on x86_64-linux, bootstraps and testsuite runs on the other platforms are still underway. What do you think? I like it, but can't approve. Ciao, Michael.
Re: [PATCH 2/3] Move MEM_REF expansion to a new function
On Tue, 13 Mar 2012, Michael Matz wrote: Hi, On Mon, 12 Mar 2012, Martin Jambor wrote: OK, the following patch changes the places where I previously called the new function to call expand_expr with EXPAND_WRITE modifier and then makes sure we do not perform reads into rvalues in expand_expr_real_1 in the contexts where I need to avoid that. So far it has passed bootstrap and testing on x86_64-linux, bootstraps and testsuite runs on the other platforms are still underway. What do you think? I like it, but can't approve. Looks good to me, too. Thus, ok for trunk if the other platform tests succeed. Thanks, Richard.
[PATCH 2/3] Move MEM_REF expansion to a new function
Hi,
when we expand a misaligned MEM_REF on the LHS, we must not call the
code in expand_expr_real_1 if the subsequent patch is applied, because
the code generates code extracting the contents of the memory to a
register, which is of course bad if the intent is to write into that
memory. Therefore expand_assignment should expand MEM_REFs itself,
just as it do when it encounters naked misaligned ones.
In order not to have nearly identical code twice in expand_assignment
and once more in expand_expr_real_1, I put it into a separate function
expand_mem_ref_to_mem_rtx (I'll be happy to change the name to
anything more correct or fitting). Nevertheless, the existing code
pieces in expand_assignment and expand_expr_real_1 sre not exactly
identical, the differences are:
- expand_expr_real_1 handles a special case when the defining
statement of the MEM_REF base is a BIT_AND_EXPR, expand_assignment
does not. The changelog introducing the change says TER
BIT_AND_EXPRs into MEM_REFs which I suspect is a good thing for
LHSs as well, so I kept the code.
- When expanding the base, the two functions differ in the
expand_modifier they pass down to expand_expr. expand_assignment
uses EXPAND_NORMAL while expand_expr_real_1 passes EXPAND_SUM.
According to the comment in expr.h the latter seemed more permissive
and so I used that, even though I admit I do not really know what
the implications of this modifier are. Is it OK to use EXPAND_SUM
also on a LHS?
- expand_expr_real_1 calls memory_address_addr_space twice, whereas
expand_assignment replaces the first call with
convert_memory_address_addr_space. Looking at the two functions I
thought it might be OK to call memory_address_addr_space (which
itself calls convert_memory_address_addr_space) only once.
But again, my expertise in this area is limited, I'll be happy to be
shown I'm wrong here.
So far I have bootstrapped and tested this patch separately on
x86_64-linx and i686-linux. Additionally, it has also passed
bootstrap and testing on usparc64-linux and ia64-linux.
Thanks in advance for any comments,
Martin
2012-03-09 Martin Jambor mjam...@suse.cz
* expr.c (expand_mem_ref_to_mem_rtx): New function.
(expand_assignment): Call it when expanding a MEM_REF on the LHS.
(expand_expr_real_1): Likewise.
Index: src/gcc/expr.c
===
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4565,6 +4565,47 @@ mem_ref_refers_to_non_mem_p (tree ref)
!MEM_P (DECL_RTL (base)));
}
+/* Expand a MEM_REF referring an object in memory to a MEM RTX. Any spacial
+ treatment of misalignment must be handled on top of the returned result. */
+
+static rtx
+expand_mem_ref_to_mem_rtx (tree ref)
+{
+ enum machine_mode address_mode, mode = TYPE_MODE (TREE_TYPE (ref));
+ tree base = TREE_OPERAND (ref, 0);
+ addr_space_t as
+= TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (ref, 0;
+ gimple def_stmt;
+ rtx mem, op0;
+
+ gcc_checking_assert (!mem_ref_refers_to_non_mem_p (ref));
+
+ address_mode = targetm.addr_space.address_mode (as);
+
+ if ((def_stmt = get_def_for_expr (base, BIT_AND_EXPR)))
+{
+ tree mask = gimple_assign_rhs2 (def_stmt);
+ base = build2 (BIT_AND_EXPR, TREE_TYPE (base),
+gimple_assign_rhs1 (def_stmt), mask);
+ TREE_OPERAND (ref, 0) = base;
+}
+ op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_SUM);
+ op0 = convert_memory_address_addr_space (address_mode, op0, as);
+ if (!integer_zerop (TREE_OPERAND (ref, 1)))
+{
+ rtx off
+= immed_double_int_const (mem_ref_offset (ref), address_mode);
+ op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
+}
+ op0 = memory_address_addr_space (mode, op0, as);
+ mem = gen_rtx_MEM (mode, op0);
+ set_mem_attributes (mem, ref, 0);
+ set_mem_addr_space (mem, as);
+ if (TREE_THIS_VOLATILE (ref))
+MEM_VOLATILE_P (mem) = 1;
+ return mem;
+}
+
/* Expand an assignment that stores the value of FROM into TO. If NONTEMPORAL
is true, try generating a nontemporal store. */
@@ -4600,46 +4641,31 @@ expand_assignment (tree to, tree from, b
!= CODE_FOR_nothing)
|| SLOW_UNALIGNED_ACCESS (mode, align)))
{
- addr_space_t as
- = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0;
struct expand_operand ops[2];
- enum machine_mode address_mode;
- rtx reg, op0, mem;
+ rtx reg, mem;
reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL);
reg = force_not_mem (reg);
if (TREE_CODE (to) == MEM_REF)
- {
- tree base = TREE_OPERAND (to, 0);
- address_mode = targetm.addr_space.address_mode (as);
- op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
- op0 = convert_memory_address_addr_space (address_mode, op0, as);
- if (!integer_zerop (TREE_OPERAND (to, 1)))
- {
-
Re: [PATCH 2/3] Move MEM_REF expansion to a new function
On Mon, 12 Mar 2012, Martin Jambor wrote: Hi, when we expand a misaligned MEM_REF on the LHS, we must not call the code in expand_expr_real_1 if the subsequent patch is applied, because the code generates code extracting the contents of the memory to a register, which is of course bad if the intent is to write into that memory. Therefore expand_assignment should expand MEM_REFs itself, just as it do when it encounters naked misaligned ones. Just a quick comment here - the expand_expr_real_1 code needs to be guarded with exactly the same conditions as the misaligned LHS case to be able to call expand_expr on it and generate a naked MEM. So if that is not working you have a bug in the RHS side handling ;) Richard.
Re: [PATCH 2/3] Move MEM_REF expansion to a new function
Hi, On Mon, 12 Mar 2012, Martin Jambor wrote: when we expand a misaligned MEM_REF on the LHS, we must not call the code in expand_expr_real_1 if the subsequent patch is applied, because the code generates code extracting the contents of the memory to a register, which is of course bad if the intent is to write into that memory. Then expand_expr_real_1 should be called with EXPAND_WRITE modifier, instead of any of the others. Then it will (or should) return an lvalue. That might still be wrong for alignment reasons, but writing into the so returned rtx will change the original object. Therefore expand_assignment should expand MEM_REFs itself, just as it do when it encounters naked misaligned ones. I think this goes into the wrong direction. expand_assignment shouldn't create an lvalue rtx for any REFs itself. It should call expand_expr with EXPAND_WRITE, and that should do the right thing (i.e. what's now done directly in expand_assignment). I realize the docu of EXPAND_WRITE is lacking, but here's what I think it should do (and what I think it actually also mostly does already): Given EXPAND_WRITE expand_expr is required to be called on an (sub)object, i.e. an lvalue, and it should return an RTX lvalue (a REG or MEM) that if written into is changing the originally specified tree lvalue (i.e. not some temporary storage). That doesn't mean that the result of expand_expr(EXPAND_WRITE) is directly usable in a simple RTL (set) pattern as LHS in all cases. For instance it won't be directly usable when it's misalign. Dealing with this situation is left to the caller, i.e. expand_assignment mostly. - When expanding the base, the two functions differ in the expand_modifier they pass down to expand_expr. expand_assignment uses EXPAND_NORMAL while expand_expr_real_1 passes EXPAND_SUM. According to the comment in expr.h the latter seemed more permissive and so I used that, even though I admit I do not really know what the implications of this modifier are. Is it OK to use EXPAND_SUM also on a LHS? No, but it might not matter in the situations you are facing, haven't checked. EXPAND_SUM can return a PLUS rtx, e.g. (plus (p60) (const_int 4)) for an offsetted address. Naturally you can't assign into such a plus rtx. But if you only expand the base with that modifier (which for BLKmode bases actually means expanding to a MEM containing the address of base) you should be fine with EXPAND_SUM, it won't be used I think. Ciao, Michael.
Re: [PATCH 2/3] Move MEM_REF expansion to a new function
Hi,
On Mon, Mar 12, 2012 at 04:26:21PM +0100, Michael Matz wrote:
Hi,
On Mon, 12 Mar 2012, Martin Jambor wrote:
when we expand a misaligned MEM_REF on the LHS, we must not call the
code in expand_expr_real_1 if the subsequent patch is applied, because
the code generates code extracting the contents of the memory to a
register, which is of course bad if the intent is to write into that
memory.
Then expand_expr_real_1 should be called with EXPAND_WRITE modifier,
instead of any of the others. Then it will (or should) return an lvalue.
That might still be wrong for alignment reasons, but writing into the so
returned rtx will change the original object.
OK, the following patch changes the places where I previously called
the new function to call expand_expr with EXPAND_WRITE modifier and
then makes sure we do not perform reads into rvalues in
expand_expr_real_1 in the contexts where I need to avoid that.
So far it has passed bootstrap and testing on x86_64-linux, bootstraps
and testsuite runs on the other platforms are still underway. What do
you think?
Thanks,
Martin
2012-03-12 Martin Jambor mjam...@suse.cz
* expr.c (expand_assignment): Use expand_expr with EXPAND_WRITE
when expanding MEM_REFs, MEM_TARGET_REFs and handled_component
bases.
(expand_expr_real_1): Do not handle misalignment if modifier is
EXPAND_WRITE.
Index: src/gcc/expr.c
===
--- src.orig/gcc/expr.c
+++ src/gcc/expr.c
@@ -4600,49 +4600,16 @@ expand_assignment (tree to, tree from, b
!= CODE_FOR_nothing)
|| SLOW_UNALIGNED_ACCESS (mode, align)))
{
- addr_space_t as
- = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0;
- struct expand_operand ops[2];
- enum machine_mode address_mode;
- rtx reg, op0, mem;
+ rtx reg, mem;
reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL);
reg = force_not_mem (reg);
-
- if (TREE_CODE (to) == MEM_REF)
- {
- tree base = TREE_OPERAND (to, 0);
- address_mode = targetm.addr_space.address_mode (as);
- op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
- op0 = convert_memory_address_addr_space (address_mode, op0, as);
- if (!integer_zerop (TREE_OPERAND (to, 1)))
- {
- rtx off
- = immed_double_int_const (mem_ref_offset (to), address_mode);
- op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
- }
- op0 = memory_address_addr_space (mode, op0, as);
- mem = gen_rtx_MEM (mode, op0);
- set_mem_attributes (mem, to, 0);
- set_mem_addr_space (mem, as);
- }
- else if (TREE_CODE (to) == TARGET_MEM_REF)
- {
- struct mem_address addr;
- get_address_description (to, addr);
- op0 = addr_for_mem_ref (addr, as, true);
- op0 = memory_address_addr_space (mode, op0, as);
- mem = gen_rtx_MEM (mode, op0);
- set_mem_attributes (mem, to, 0);
- set_mem_addr_space (mem, as);
- }
- else
- gcc_unreachable ();
- if (TREE_THIS_VOLATILE (to))
- MEM_VOLATILE_P (mem) = 1;
+ mem = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE);
if (icode != CODE_FOR_nothing)
{
+ struct expand_operand ops[2];
+
create_fixed_operand (ops[0], mem);
create_input_operand (ops[1], reg, mode);
/* The movmisalignmode pattern cannot fail, else the assignment
@@ -4695,31 +4662,11 @@ expand_assignment (tree to, tree from, b
((icode = optab_handler (movmisalign_optab, mode))
!= CODE_FOR_nothing))
{
- enum machine_mode address_mode;
- rtx op0;
struct expand_operand ops[2];
- addr_space_t as = TYPE_ADDR_SPACE
- (TREE_TYPE (TREE_TYPE (TREE_OPERAND (tem, 0;
- tree base = TREE_OPERAND (tem, 0);
misalignp = true;
to_rtx = gen_reg_rtx (mode);
-
- address_mode = targetm.addr_space.address_mode (as);
- op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
- op0 = convert_memory_address_addr_space (address_mode, op0, as);
- if (!integer_zerop (TREE_OPERAND (tem, 1)))
- {
- rtx off = immed_double_int_const (mem_ref_offset (tem),
- address_mode);
- op0 = simplify_gen_binary (PLUS, address_mode, op0, off);
- }
- op0 = memory_address_addr_space (mode, op0, as);
- mem = gen_rtx_MEM (mode, op0);
- set_mem_attributes (mem, tem, 0);
- set_mem_addr_space (mem, as);
- if (TREE_THIS_VOLATILE (tem))
- MEM_VOLATILE_P (mem) = 1;
+ mem = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
/* If the misaligned store doesn't overwrite all bits, perform
