Re: [pooma-dev] [PATCH] Fix PackUnpack bug

[Richard Guenther]

On Tue, 24 Aug 2004, Jeffrey D. Oldham wrote:

> Richard Guenther wrote:
>
> >PackUnpack.h pack/unpack use a functor with LoopApplyEvaluator
> >that violate the assumption of independent iterations.  Thus
> >the Field/LocalPatch testcase fails for OpenMP.
> >
> >We have the required infrastructure for ordered LoopApply in
> >RemoteEngine.h.  This patch uses that, but before needs to fix
> >it as it works with zeroBased domain only due to a bug.
> >
> >
> I do not understand this last sentence.  I also do not understand the
> patch, mainly because of my ignorance.
>
> Each portion of the patch is internally consistent, but why should a
> local patch ignore its field?

I'll try to go through the patch with some explanations

> >Index: Engine/RemoteEngine.h
> >===================================================================
> >RCS file: /home/pooma/Repository/r2/src/Engine/RemoteEngine.h,v
> >retrieving revision 1.42
> >diff -u -u -r1.42 RemoteEngine.h
> >--- Engine/RemoteEngine.h    19 Jan 2004 22:04:33 -0000      1.42
> >+++ Engine/RemoteEngine.h    24 Aug 2004 14:23:53 -0000
> >@@ -1055,8 +1055,8 @@
> >     {
> >       CTAssert(Domain::unitStride == 1);
> >       int f0 = domain[0].first();
> >-      int e0 = domain[0].length();
> >-      for (int i0 = f0; i0<e0; ++i0)
> >+      int e0 = domain[0].last();
> >+      for (int i0 = f0; i0<=e0; ++i0)
> >     op(engine(i0));
> >       return op.total_m;
> >     }

This was clearly wrong.  Either it should go from zero to
domain[0].length() or from domain[0].first() to domain[0].last().  We
didn't catch this yet as the past use was with domain[0].first() == 0
always.

> >Index: Functions/PackUnpack.h
> >===================================================================
> >RCS file: /home/pooma/Repository/r2/src/Functions/PackUnpack.h,v
> >retrieving revision 1.5
> >diff -u -u -r1.5 PackUnpack.h
> >--- Functions/PackUnpack.h   25 Oct 2003 12:06:55 -0000      1.5
> >+++ Functions/PackUnpack.h   24 Aug 2004 14:23:53 -0000
> >@@ -59,6 +59,7 @@
> > //-----------------------------------------------------------------------------
> >
> > #include "Utilities/RefCountedBlockPtr.h"
> >+#include "Engine/RemoteEngine.h"
> > #include "Pooma/Pooma.h"
> >
> > //-----------------------------------------------------------------------------
> >@@ -93,38 +94,19 @@
> > {
> >   typedef typename InputField::Element_t Element_t;
> >
> >-  PackLocalPatches(const InputField &field,
> >-                   RefCountedBlockPtr<Element_t> block)
> >-    : field_m(field), block_m(block)
> >+  PackLocalPatches(RefCountedBlockPtr<Element_t> block)
> >+    : block_m(block)
> >   {
> >   }
> >
> >-  void operator()(int i0) const
> >+  inline void operator()(const Element_t &t)
> >   {
> >-    *block_m = field_m.read(i0);
> >+    *block_m = t;
> >     ++block_m;
> >   }
> >
> >-  void operator()(int i0, int i1) const
> >-  {
> >-    *block_m = field_m.read(i0, i1);
> >-    ++block_m;
> >-  }
> >-
> >-  void operator()(int i0, int i1, int i2) const
> >-  {
> >-    *block_m = field_m.read(i0, i1, i2);
> >-    ++block_m;
> >-  }
> >-
> >-  void operator()(int i0, int i1, int i2, int i3) const
> >-  {
> >-    *block_m = field_m.read(i0, i1, i2, i3);
> >-    ++block_m;
> >-  }
> >-
> >-  InputField field_m;
> >-  mutable RefCountedBlockPtr<Element_t> block_m;
> >+  RefCountedBlockPtr<Element_t> block_m;
> >+  int total_m;
> > };

Rewrite the functor to work with EngineBlockSerialize instead of
LoopApplyEvaluator.

> > template<class InputField>
> >@@ -149,8 +131,8 @@
> >   {
> >     typedef typename Patch<InputField>::Type_t PatchField_t;
> >     PatchField_t patch = field.patchLocal(i);
> >-    PackLocalPatches<PatchField_t> packFunctor(patch, current);
> >-    LoopApplyEvaluator::evaluate(packFunctor, patch.domain());
> >+    PackLocalPatches<PatchField_t> packFunctor(current);
> >+    EngineBlockSerialize::apply(packFunctor, patch, patch.domain());
> >     current += patch.domain().size();
> >   }

Use EngineBlockSerialize with the PackLocalPatches functor.
EngineBlockSerialize passes the read value to the functor, not the
current index as LoopApplyEvaluator does.  So we don't need the local
patch in the functor, but only in EngineBlockSerialize.

And likewise for Pack.

The problem with OpenMP and LoopApplyEvaluator was that the loop
for that evaluator is OpenMP parallelized - and guess what happens
to the block_m member of the functor if it's incremented and written
to by multiple threads in parallel...

Richard.

--
Richard Guenther <richard dot guenther at uni-tuebingen dot de>
WWW: http://www.tat.physik.uni-tuebingen.de/~rguenth/