[PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Jakub Jelinek]

Hi!

Now that Richi has fixed up SRA not to pessimize code by changing non-BLK
mode arguments into their BLKmode subparts, I think it would be nice
to fix up also the expansion of the BLKmode MEM_REFs that have non-BLKmode
non-addressable base decl.  While this doesn't happen for this testcase
anymore, it rarely still occurs and I think it is wrong to expand
a BLKmode MEM_REF into a non-BLKmode rtx.  While the callers sometimes
can cope with it, often they can't.

Bootstrapped/regtested on x86_64-linux and i686-linux, bootstrapped on
powerpc64-linux (both --with-cpu=default32 and with real 64-bit
profiledbootstrap).  Ok for trunk?

2012-01-19  Jakub Jelinek  <jakub@redhat.com>

	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.

--- gcc/expr.c.jj	2012-01-13 21:47:35.000000000 +0100
+++ gcc/expr.c	2012-01-19 13:12:14.218760812 +0100
@@ -9328,6 +9328,16 @@ expand_expr_real_1 (tree exp, rtx target
 		bftype = TREE_TYPE (base);
 		if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode)
 		  bftype = TREE_TYPE (exp);
+		else
+		  {
+		    temp = assign_stack_temp (DECL_MODE (base),
+					      GET_MODE_SIZE (DECL_MODE (base)),
+					      0);
+		    store_expr (base, temp, 0, false);
+		    temp = adjust_address (temp, BLKmode, offset);
+		    set_mem_size (temp, int_size_in_bytes (TREE_TYPE (exp)));
+		    return temp;
+		  }
 		return expand_expr (build3 (BIT_FIELD_REF, bftype,
 					    base,
 					    TYPE_SIZE (TREE_TYPE (exp)),
--- gcc/testsuite/g++.dg/opt/pr51895.C.jj	2012-01-19 13:20:27.808899825 +0100
+++ gcc/testsuite/g++.dg/opt/pr51895.C	2012-01-19 13:21:10.042655293 +0100
@@ -0,0 +1,25 @@
+// PR middle-end/51895
+// { dg-do compile }
+// { dg-options "-O2" }
+
+struct S
+{
+  long a;
+  char b;
+  S () : a (0), b (0) {}
+  bool baz ();
+};
+
+__attribute__((noinline)) static bool
+bar (S x, S y)
+{
+  y = x;
+  return y.baz ();
+}
+
+bool
+foo (S x)
+{
+  S y;
+  return bar (x, y);
+}

	Jakub

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Eric Botcazou]

> Now that Richi has fixed up SRA not to pessimize code by changing non-BLK
> mode arguments into their BLKmode subparts, I think it would be nice
> to fix up also the expansion of the BLKmode MEM_REFs that have non-BLKmode
> non-addressable base decl.  While this doesn't happen for this testcase
> anymore, it rarely still occurs and I think it is wrong to expand
> a BLKmode MEM_REF into a non-BLKmode rtx.  While the callers sometimes
> can cope with it, often they can't.

Another example is PR middle-end/51959, the BLKmode MEM_REF with non-BLKmode 
base is created by inlining, expanded to a DImode REG in expand_assignment and 
then sent to store_field, which attemps to spill it to memory but runs into an 
alias set conflict.  I'll try to redirect it to the regular MEM_REF expander 
instead, so your patch might be a prerequisite.

> Bootstrapped/regtested on x86_64-linux and i686-linux, bootstrapped on
> powerpc64-linux (both --with-cpu=default32 and with real 64-bit
> profiledbootstrap).  Ok for trunk?
>
> 2012-01-19  Jakub Jelinek  <jakub@redhat.com>
>
> 	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.

I think that it should be applied.

-- 
Eric Botcazou

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Thu, 26 Jan 2012, Eric Botcazou wrote:

> > Now that Richi has fixed up SRA not to pessimize code by changing non-BLK
> > mode arguments into their BLKmode subparts, I think it would be nice
> > to fix up also the expansion of the BLKmode MEM_REFs that have non-BLKmode
> > non-addressable base decl.  While this doesn't happen for this testcase
> > anymore, it rarely still occurs and I think it is wrong to expand
> > a BLKmode MEM_REF into a non-BLKmode rtx.  While the callers sometimes
> > can cope with it, often they can't.
> 
> Another example is PR middle-end/51959, the BLKmode MEM_REF with non-BLKmode 
> base is created by inlining, expanded to a DImode REG in expand_assignment and 
> then sent to store_field, which attemps to spill it to memory but runs into an 
> alias set conflict.  I'll try to redirect it to the regular MEM_REF expander 
> instead, so your patch might be a prerequisite.

Hmm, how can store_field run into an "alias set conflict"?  ...

Richard.

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Eric Botcazou]

> Hmm, how can store_field run into an "alias set conflict"?  ...

Bad wording... I meant an alias set issue.  The two alias sets (that of the 
BLKmode type and that of the non-BLKmode type) precisely don't conflict when 
store_field does its spilling trick (third "if" in the function) so it stops.

-- 
Eric Botcazou

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Thu, 26 Jan 2012, Richard Guenther wrote:

> On Thu, 26 Jan 2012, Eric Botcazou wrote:
> 
> > > Now that Richi has fixed up SRA not to pessimize code by changing non-BLK
> > > mode arguments into their BLKmode subparts, I think it would be nice
> > > to fix up also the expansion of the BLKmode MEM_REFs that have non-BLKmode
> > > non-addressable base decl.  While this doesn't happen for this testcase
> > > anymore, it rarely still occurs and I think it is wrong to expand
> > > a BLKmode MEM_REF into a non-BLKmode rtx.  While the callers sometimes
> > > can cope with it, often they can't.
> > 
> > Another example is PR middle-end/51959, the BLKmode MEM_REF with non-BLKmode 
> > base is created by inlining, expanded to a DImode REG in expand_assignment and 
> > then sent to store_field, which attemps to spill it to memory but runs into an 
> > alias set conflict.  I'll try to redirect it to the regular MEM_REF expander 
> > instead, so your patch might be a prerequisite.
> 
> Hmm, how can store_field run into an "alias set conflict"?  ...

I think the bug is that

  if (mode == BLKmode
      && (REG_P (target) || GET_CODE (target) == SUBREG))
    {
      rtx object = assign_temp (type, 0, 1, 1);
      rtx blk_object = adjust_address (object, BLKmode, 0);

assign_temp here uses TYPE which is not compatible with the
requested ALIAS_SET.  expand_assignment calls it like

            result = store_field (to_rtx, bitsize, bitpos,
                                  bitregion_start, bitregion_end,
                                  mode1, from,
                                  TREE_TYPE (tem), get_alias_set (to),
                                  nontemporal);

but that of course does not work for view-converted to (thus,
a MEM_REF on a placement-new accessed storage for example).

We need to simply force ALIAS_SET to be set on the assign_temp
result - but it looks like that's not easily possible without
hitting that very same assert.  Ugh.

Of course get_inner_reference looks through these kind of
conversions when returning the ultimate decl in MEM [&foo],
see the jumps in tree-ssa-alias.c we perform to re-discover
the view-converting cases ... (at some point I realized that
this probably wasn't the best design decision).  So maybe
if the passed type isn't used in any other way we can
get around and discover the view-convert and use the alias-type
of the MEM_REF ...

Richard.

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Eric Botcazou]

> Of course get_inner_reference looks through these kind of
> conversions when returning the ultimate decl in MEM [&foo],
> see the jumps in tree-ssa-alias.c we perform to re-discover
> the view-converting cases ... (at some point I realized that
> this probably wasn't the best design decision).  So maybe
> if the passed type isn't used in any other way we can
> get around and discover the view-convert and use the alias-type
> of the MEM_REF ...

But I presume that the regular MEM_REF expander can cope with this case?

-- 
Eric Botcazou

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Thu, 26 Jan 2012, Eric Botcazou wrote:

> > Of course get_inner_reference looks through these kind of
> > conversions when returning the ultimate decl in MEM [&foo],
> > see the jumps in tree-ssa-alias.c we perform to re-discover
> > the view-converting cases ... (at some point I realized that
> > this probably wasn't the best design decision).  So maybe
> > if the passed type isn't used in any other way we can
> > get around and discover the view-convert and use the alias-type
> > of the MEM_REF ...
> 
> But I presume that the regular MEM_REF expander can cope with this case?

Sure.

Btw, we seem to use the TYPE argument solely for the purpose of
the assign_temp call - and in the forwarding to store_field
we pass down the very same alias_set which isn't needed,
we can just use MEM_ALIAS_SET (blk_object) here it seems,
it's different memory after all, no need to conflict with TARGET
(or set MEM_KEEP_ALIAS_SET_P - what's that ..., ah, DECL_NONADDRESSABLE 
...).

Of course if you can simplify the code by using the regular
expander all the better (and eliminate the TYPE argument?).

@@ -6299,7 +6302,7 @@ store_field (rtx target, HOST_WIDE_INT b
 
       store_field (blk_object, bitsize, bitpos,
                   bitregion_start, bitregion_end,
-                  mode, exp, type, alias_set, nontemporal);
+                  mode, exp, type, MEM_ALIAS_SET (blk_object), 
nontemporal);
 
       emit_move_insn (target, object);
 

works for me.

Thanks,
Richard.

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Thu, 26 Jan 2012, Richard Guenther wrote:

> On Thu, 26 Jan 2012, Eric Botcazou wrote:
> 
> > > Of course get_inner_reference looks through these kind of
> > > conversions when returning the ultimate decl in MEM [&foo],
> > > see the jumps in tree-ssa-alias.c we perform to re-discover
> > > the view-converting cases ... (at some point I realized that
> > > this probably wasn't the best design decision).  So maybe
> > > if the passed type isn't used in any other way we can
> > > get around and discover the view-convert and use the alias-type
> > > of the MEM_REF ...
> > 
> > But I presume that the regular MEM_REF expander can cope with this case?
> 
> Sure.
> 
> Btw, we seem to use the TYPE argument solely for the purpose of
> the assign_temp call - and in the forwarding to store_field
> we pass down the very same alias_set which isn't needed,
> we can just use MEM_ALIAS_SET (blk_object) here it seems,
> it's different memory after all, no need to conflict with TARGET
> (or set MEM_KEEP_ALIAS_SET_P - what's that ..., ah, DECL_NONADDRESSABLE 
> ...).
> 
> Of course if you can simplify the code by using the regular
> expander all the better (and eliminate the TYPE argument?).
> 
> @@ -6299,7 +6302,7 @@ store_field (rtx target, HOST_WIDE_INT b
>  
>        store_field (blk_object, bitsize, bitpos,
>                    bitregion_start, bitregion_end,
> -                  mode, exp, type, alias_set, nontemporal);
> +                  mode, exp, type, MEM_ALIAS_SET (blk_object), 
> nontemporal);
>  
>        emit_move_insn (target, object);
>  
> 
> works for me.

So I am testing the following - please tell me whether you are working
on a different fix.

Thanks,
Richard.

2012-01-27  Richard Guenther  <rguenther@suse.de>

	PR middle-end/51959
	* expr.c (store_field): Use the alias-set of the scratch memory
	for storing to it.

	* g++.dg/torture/pr51959.C: New testcase.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 183606)
+++ gcc/expr.c	(working copy)
@@ -6299,7 +6302,7 @@ store_field (rtx target, HOST_WIDE_INT b
 
       store_field (blk_object, bitsize, bitpos,
 		   bitregion_start, bitregion_end,
-		   mode, exp, type, alias_set, nontemporal);
+		   mode, exp, type, MEM_ALIAS_SET (blk_object), nontemporal);
 
       emit_move_insn (target, object);
 

Index: gcc/testsuite/g++.dg/torture/pr51959.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr51959.C	(revision 0)
+++ gcc/testsuite/g++.dg/torture/pr51959.C	(revision 0)
@@ -0,0 +1,80 @@
+// { dg-do compile }
+
+namespace std {
+    typedef __SIZE_TYPE__ size_t;
+}
+inline void* operator new(std::size_t, void* __p) throw() {
+    return __p;
+}
+template <typename T> class QTypeInfo {
+};
+enum { Q_COMPLEX_TYPE = 0,     Q_PRIMITIVE_TYPE = 0x1,     Q_STATIC_TYPE = 0,     Q_MOVABLE_TYPE = 0x2,     Q_DUMMY_TYPE = 0x4 };
+template<typename Enum> class QFlags {
+    int i;
+    inline QFlags(Enum f) : i(f) { }
+};
+class __attribute__((visibility("default"))) QSize {
+public:
+    bool isEmpty() const;
+    friend inline bool operator==(const QSize &, const QSize &);
+    int wd;
+    int ht;
+};
+template<> class QTypeInfo<QSize > {
+public:
+    enum {
+	isComplex = (((Q_MOVABLE_TYPE) & Q_PRIMITIVE_TYPE) == 0), isStatic = (((Q_MOVABLE_TYPE) & (Q_MOVABLE_TYPE | Q_PRIMITIVE_TYPE)) == 0), isLarge = (sizeof(QSize)>sizeof(void*)), isPointer = false, isDummy = (((Q_MOVABLE_TYPE) & Q_DUMMY_TYPE) != 0) };
+};
+class __attribute__((visibility("default"))) QBasicAtomicInt {
+public:
+    inline bool operator!=(int value) const     { }
+};
+struct __attribute__((visibility("default"))) QListData {
+    struct Data {
+	QBasicAtomicInt ref;
+    };
+    void **append();
+};
+template <typename T> class QList {
+    struct Node {
+	void *v;
+    };
+    union {
+	QListData p;
+	QListData::Data *d;
+    };
+public:
+    void append(const T &t);
+    inline void push_back(const T &t) {
+	append(t);
+    }
+    void node_construct(Node *n, const T &t);
+    void node_destruct(Node *n);
+};
+template <typename T> inline void QList<T>::node_construct(Node *n, const T &t) {
+    if (QTypeInfo<T>::isLarge || QTypeInfo<T>::isStatic) n->v = new T(t);
+    else if (QTypeInfo<T>::isComplex) new (n) T(t);
+}
+template <typename T> inline void QList<T>::node_destruct(Node *n) {
+}
+template <typename T>  void QList<T>::append(const T &t) {
+    if (d->ref != 1) {
+	try {
+	}
+	catch (...) {
+	}
+	if (QTypeInfo<T>::isLarge || QTypeInfo<T>::isStatic) {
+	}
+	else {
+	    Node *n, copy;
+	    node_construct(&copy, t);
+	    try {                 n = reinterpret_cast<Node *>(p.append());;             }
+	    catch (...) {                 node_destruct(&copy);                 throw;             }
+	    *n = copy;
+	}
+    }
+};
+void virtual_hook(QSize sz, QList<QSize> &arg)
+{
+  arg.push_back(sz);
+}

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Eric Botcazou]

> So I am testing the following - please tell me whether you are working
> on a different fix.

I was, but I realized that this would somewhat conflict with your latest patch 
to expand_assignment, where all MEM_REFs will go through get_inner_reference 
instead of the regular expander.

Can't we avoid doing this if they have BLKmode?  Because you cannot get a 
BLKmode RTX out of this if the base hasn't BLKmode.  We would need to spill, 
like in the regular expander, so we might as well use it.

Otherwise, you're the resident expert in aliasing so, if you think that your 
patchlet is sufficient, fine with me.

-- 
Eric Botcazou

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Fri, 27 Jan 2012, Eric Botcazou wrote:

> > So I am testing the following - please tell me whether you are working
> > on a different fix.
> 
> I was, but I realized that this would somewhat conflict with your latest patch 
> to expand_assignment, where all MEM_REFs will go through get_inner_reference 
> instead of the regular expander.
>
> Can't we avoid doing this if they have BLKmode?  Because you cannot get a 
> BLKmode RTX out of this if the base hasn't BLKmode.  We would need to spill, 
> like in the regular expander, so we might as well use it.

I was simply trying to save some code-duplication here.  I will
re-work the patch to avoid this as you suggest.  Btw, the original
reason why we handle MEM_REF at all via the get_inner_reference
path is that if we have MEM_REF[&decl, CST] and decl was not
committed to memory then the MEM_REF really acts like a
bitfield insert to a non-MEM (similar to the rvalue case we
handle in expand_expr_real_1).

I suppose I should split out some predicates that can be shared
amongst the various TARGET_[MEM_REF] handlers during expansion
and tighten this one up as well.

> Otherwise, you're the resident expert in aliasing so, if you think that your 
> patchlet is sufficient, fine with me.

Yes, I think it is sufficient for this case.

Now, let me rework that expand_assignment patch a bit.

Richard.

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Fri, 27 Jan 2012, Richard Guenther wrote:

> On Fri, 27 Jan 2012, Eric Botcazou wrote:
> 
> > > So I am testing the following - please tell me whether you are working
> > > on a different fix.
> > 
> > I was, but I realized that this would somewhat conflict with your latest patch 
> > to expand_assignment, where all MEM_REFs will go through get_inner_reference 
> > instead of the regular expander.
> >
> > Can't we avoid doing this if they have BLKmode?  Because you cannot get a 
> > BLKmode RTX out of this if the base hasn't BLKmode.  We would need to spill, 
> > like in the regular expander, so we might as well use it.
> 
> I was simply trying to save some code-duplication here.  I will
> re-work the patch to avoid this as you suggest.  Btw, the original
> reason why we handle MEM_REF at all via the get_inner_reference
> path is that if we have MEM_REF[&decl, CST] and decl was not
> committed to memory then the MEM_REF really acts like a
> bitfield insert to a non-MEM (similar to the rvalue case we
> handle in expand_expr_real_1).
> 
> I suppose I should split out some predicates that can be shared
> amongst the various TARGET_[MEM_REF] handlers during expansion
> and tighten this one up as well.
> 
> > Otherwise, you're the resident expert in aliasing so, if you think that your 
> > patchlet is sufficient, fine with me.
> 
> Yes, I think it is sufficient for this case.
> 
> Now, let me rework that expand_assignment patch a bit.

Like the following, bootstrapped and tested on x86_64-unknown-linux-gnu.
I'll apply it later unless I hear some objections.

Thanks,
Richard.

2012-01-27  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/50444
	* expr.c (mem_ref_refers_to_non_mem_p): New function.
	(expand_assignment): Use it.  Properly handle misaligned
	bases when expanding stores to component references.
	(expand_expr_real_1): Use mem_ref_refers_to_non_mem_p and
	refactor that case.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 183616)
+++ gcc/expr.c	(working copy)
@@ -4548,6 +4548,23 @@ get_bit_range (unsigned HOST_WIDE_INT *b
     }
 }
 
+/* Returns true if the MEM_REF REF refers to an object that does not
+   reside in memory and has non-BLKmode.  */
+
+static bool
+mem_ref_refers_to_non_mem_p (tree ref)
+{
+  tree base = TREE_OPERAND (ref, 0);
+  if (TREE_CODE (base) != ADDR_EXPR)
+    return false;
+  base = TREE_OPERAND (base, 0);
+  return (DECL_P (base)
+	  && !TREE_ADDRESSABLE (base)
+	  && DECL_MODE (base) != BLKmode
+	  && DECL_RTL_SET_P (base)
+	  && !MEM_P (DECL_RTL (base)));
+}
+
 /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
    is true, try generating a nontemporal store.  */
 
@@ -4571,6 +4588,7 @@ expand_assignment (tree to, tree from, b
   if (operand_equal_p (to, from, 0))
     return;
 
+  /* Handle misaligned stores.  */
   mode = TYPE_MODE (TREE_TYPE (to));
   if ((TREE_CODE (to) == MEM_REF
        || TREE_CODE (to) == TARGET_MEM_REF)
@@ -4580,6 +4598,8 @@ expand_assignment (tree to, tree from, b
       && ((icode = optab_handler (movmisalign_optab, mode))
 	  != CODE_FOR_nothing))
     {
+      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;
@@ -4589,8 +4609,6 @@ expand_assignment (tree to, tree from, b
 
       if (TREE_CODE (to) == MEM_REF)
 	{
-	  addr_space_t as
-	    = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
 	  tree base = TREE_OPERAND (to, 0);
 	  address_mode = targetm.addr_space.address_mode (as);
 	  op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_NORMAL);
@@ -4598,7 +4616,7 @@ expand_assignment (tree to, tree from, b
 	  if (!integer_zerop (TREE_OPERAND (to, 1)))
 	    {
 	      rtx off
-		  = immed_double_int_const (mem_ref_offset (to), address_mode);
+		= 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);
@@ -4608,10 +4626,7 @@ expand_assignment (tree to, tree from, b
 	}
       else if (TREE_CODE (to) == TARGET_MEM_REF)
 	{
-	  addr_space_t as
-	    = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (to, 0))));
 	  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);
@@ -4627,7 +4642,7 @@ expand_assignment (tree to, tree from, b
       create_fixed_operand (&ops[0], mem);
       create_input_operand (&ops[1], reg, mode);
       /* The movmisalign<mode> pattern cannot fail, else the assignment would
-         silently be omitted.  */
+	 silently be omitted.  */
       expand_insn (icode, 2, ops);
       return;
     }
@@ -4636,12 +4651,10 @@ expand_assignment (tree to, tree from, b
      if the structure component's rtx is not simply a MEM.
      Assignment of an array element at a constant index, and assignment of
      an array element in an unaligned packed structure field, has the same
-     problem.  */
+     problem.  Same for (partially) storing into a non-memory object.  */
   if (handled_component_p (to)
-      /* ???  We only need to handle MEM_REF here if the access is not
-         a full access of the base object.  */
       || (TREE_CODE (to) == MEM_REF
-	  && TREE_CODE (TREE_OPERAND (to, 0)) == ADDR_EXPR)
+	  && mem_ref_refers_to_non_mem_p (to))
       || TREE_CODE (TREE_TYPE (to)) == ARRAY_TYPE)
     {
       enum machine_mode mode1;
@@ -4652,6 +4665,7 @@ expand_assignment (tree to, tree from, b
       int unsignedp;
       int volatilep = 0;
       tree tem;
+      bool misalignp;
 
       push_temp_slots ();
       tem = get_inner_reference (to, &bitsize, &bitpos, &offset, &mode1,
@@ -4664,8 +4678,22 @@ expand_assignment (tree to, tree from, b
 
       /* If we are going to use store_bit_field and extract_bit_field,
 	 make sure to_rtx will be safe for multiple use.  */
-
-      to_rtx = expand_normal (tem);
+      mode = TYPE_MODE (TREE_TYPE (tem));
+      if (TREE_CODE (tem) == MEM_REF
+	  && mode != BLKmode
+	  && ((align = get_object_or_type_alignment (tem))
+	      < GET_MODE_ALIGNMENT (mode))
+	  && ((icode = optab_handler (movmisalign_optab, mode))
+	      != CODE_FOR_nothing))
+	{
+	  misalignp = true;
+	  to_rtx = gen_reg_rtx (mode);
+	}
+      else
+	{
+	  misalignp = false;
+	  to_rtx = expand_normal (tem);
+	}
 
       /* If the bitfield is volatile, we want to access it in the
 	 field's mode, not the computed mode.
@@ -4811,6 +4839,37 @@ expand_assignment (tree to, tree from, b
 				  nontemporal);
 	}
 
+      if (misalignp)
+	{
+	  struct expand_operand ops[2];
+	  enum machine_mode address_mode;
+	  rtx op0, mem;
+	  addr_space_t as = TYPE_ADDR_SPACE
+	      (TREE_TYPE (TREE_TYPE (TREE_OPERAND (tem, 0))));
+	  tree base = TREE_OPERAND (tem, 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 (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;
+
+	  create_fixed_operand (&ops[0], mem);
+	  create_input_operand (&ops[1], to_rtx, mode);
+	  /* The movmisalign<mode> pattern cannot fail, else the assignment
+	     would silently be omitted.  */
+	  expand_insn (icode, 2, ops);
+	}
+
       if (result)
 	preserve_temp_slots (result);
       free_temp_slots ();
@@ -4866,11 +4925,8 @@ expand_assignment (tree to, tree from, b
       return;
     }
 
-  /* Ordinary treatment.  Expand TO to get a REG or MEM rtx.
-     Don't re-expand if it was expanded already (in COMPONENT_REF case).  */
-
-  if (to_rtx == 0)
-    to_rtx = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE);
+  /* Ordinary treatment.  Expand TO to get a REG or MEM rtx.  */
+  to_rtx = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
   /* Don't move directly into a return register.  */
   if (TREE_CODE (to) == RESULT_DECL
@@ -9295,54 +9351,38 @@ expand_expr_real_1 (tree exp, rtx target
 	unsigned align;
 	/* Handle expansion of non-aliased memory with non-BLKmode.  That
 	   might end up in a register.  */
-	if (TREE_CODE (base) == ADDR_EXPR)
+	if (mem_ref_refers_to_non_mem_p (exp))
 	  {
 	    HOST_WIDE_INT offset = mem_ref_offset (exp).low;
 	    tree bit_offset;
+	    tree bftype;
 	    base = TREE_OPERAND (base, 0);
-	    if (!DECL_P (base))
-	      {
-		HOST_WIDE_INT off;
-		base = get_addr_base_and_unit_offset (base, &off);
-		gcc_assert (base);
-		offset += off;
-	      }
-	    /* If we are expanding a MEM_REF of a non-BLKmode non-addressable
-	       decl we must use bitfield operations.  */
-	    if (DECL_P (base)
-		&& !TREE_ADDRESSABLE (base)
-		&& DECL_MODE (base) != BLKmode
-		&& DECL_RTL_SET_P (base)
-		&& !MEM_P (DECL_RTL (base)))
+	    if (offset == 0
+		&& host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
+		&& (GET_MODE_BITSIZE (DECL_MODE (base))
+		    == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
+	      return expand_expr (build1 (VIEW_CONVERT_EXPR,
+					  TREE_TYPE (exp), base),
+				  target, tmode, modifier);
+	    bit_offset = bitsize_int (offset * BITS_PER_UNIT);
+	    bftype = TREE_TYPE (base);
+	    if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode)
+	      bftype = TREE_TYPE (exp);
+	    else
 	      {
-		tree bftype;
-		if (offset == 0
-		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
-		    && (GET_MODE_BITSIZE (DECL_MODE (base))
-			== TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
-		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
-					      TREE_TYPE (exp), base),
-				      target, tmode, modifier);
-		bit_offset = bitsize_int (offset * BITS_PER_UNIT);
-		bftype = TREE_TYPE (base);
-		if (TYPE_MODE (TREE_TYPE (exp)) != BLKmode)
-		  bftype = TREE_TYPE (exp);
-		else
-		  {
-		    temp = assign_stack_temp (DECL_MODE (base),
-					      GET_MODE_SIZE (DECL_MODE (base)),
-					      0);
-		    store_expr (base, temp, 0, false);
-		    temp = adjust_address (temp, BLKmode, offset);
-		    set_mem_size (temp, int_size_in_bytes (TREE_TYPE (exp)));
-		    return temp;
-		  }
-		return expand_expr (build3 (BIT_FIELD_REF, bftype,
-					    base,
-					    TYPE_SIZE (TREE_TYPE (exp)),
-					    bit_offset),
-				    target, tmode, modifier);
+		temp = assign_stack_temp (DECL_MODE (base),
+					  GET_MODE_SIZE (DECL_MODE (base)),
+					  0);
+		store_expr (base, temp, 0, false);
+		temp = adjust_address (temp, BLKmode, offset);
+		set_mem_size (temp, int_size_in_bytes (TREE_TYPE (exp)));
+		return temp;
 	      }
+	    return expand_expr (build3 (BIT_FIELD_REF, bftype,
+					base,
+					TYPE_SIZE (TREE_TYPE (exp)),
+					bit_offset),
+				target, tmode, modifier);
 	  }
 	address_mode = targetm.addr_space.address_mode (as);
 	base = TREE_OPERAND (exp, 0);

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Richard Guenther]

On Fri, 27 Jan 2012, Richard Guenther wrote:

> On Fri, 27 Jan 2012, Richard Guenther wrote:
> 
> > On Fri, 27 Jan 2012, Eric Botcazou wrote:
> > 
> > > > So I am testing the following - please tell me whether you are working
> > > > on a different fix.
> > > 
> > > I was, but I realized that this would somewhat conflict with your latest patch 
> > > to expand_assignment, where all MEM_REFs will go through get_inner_reference 
> > > instead of the regular expander.
> > >
> > > Can't we avoid doing this if they have BLKmode?  Because you cannot get a 
> > > BLKmode RTX out of this if the base hasn't BLKmode.  We would need to spill, 
> > > like in the regular expander, so we might as well use it.
> > 
> > I was simply trying to save some code-duplication here.  I will
> > re-work the patch to avoid this as you suggest.  Btw, the original
> > reason why we handle MEM_REF at all via the get_inner_reference
> > path is that if we have MEM_REF[&decl, CST] and decl was not
> > committed to memory then the MEM_REF really acts like a
> > bitfield insert to a non-MEM (similar to the rvalue case we
> > handle in expand_expr_real_1).
> > 
> > I suppose I should split out some predicates that can be shared
> > amongst the various TARGET_[MEM_REF] handlers during expansion
> > and tighten this one up as well.
> > 
> > > Otherwise, you're the resident expert in aliasing so, if you think that your 
> > > patchlet is sufficient, fine with me.
> > 
> > Yes, I think it is sufficient for this case.
> > 
> > Now, let me rework that expand_assignment patch a bit.
> 
> Like the following, bootstrapped and tested on x86_64-unknown-linux-gnu.
> I'll apply it later unless I hear some objections.

I have applied this now and the SRA side of the fix for PR50444.
As far as I know x86 should be ok with alignment-related issues
and SRA now - is there still a bug on strict-alignment platforms
we know of (thus, with a testcase)?  Otherwise I'd rather delay
trying to properly expand misaligned MEM_REFs on non-movmisalign
STRICT_ALIGNMENT targets to 4.8.  I still want to deal with 51528
though.

Thanks,
Richard.

Re: [PATCH] Fix expansion of BLKmode MEM_REF with non-addressable non-BLKmode base decl (PR middle-end/51895)

[Eric Botcazou]

> I have applied this now and the SRA side of the fix for PR50444.
> As far as I know x86 should be ok with alignment-related issues
> and SRA now - is there still a bug on strict-alignment platforms
> we know of (thus, with a testcase)?

Not that I know of, but let me do some testing on the SPARC this week-end.

> Otherwise I'd rather delay trying to properly expand misaligned MEM_REFs on
> non-movmisalign STRICT_ALIGNMENT targets to 4.8.

That sounds sensible to me.

-- 
Eric Botcazou