diff mbox

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

Message ID alpine.LNX.2.00.1201271034440.4999@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Jan. 27, 2012, 9:35 a.m. UTC 
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.

Comments

Eric Botcazou Jan. 27, 2012, 10:09 a.m. UTC | #1 
> 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.
Richard Biener Jan. 27, 2012, 10:20 a.m. UTC | #2 
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.
diff mbox

Patch

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);
+}