[Beignet] [PATCH] Fix a random assert in scalarize pass.

[Zhigang Gong]

Hi Rong,

This fix may not be totally correct. As per your description, one fake value
may be deleted after
the first scalarize pass. And the scalarize is not aware of that change. So
there is a potential
bug hide here. As the scalarize doesn't remove the related instructions, so
latter it may still
try to translate that instruction to gen IR. Then when it try to get the
removed register, it
should fail. And that failure should be also a random failure, as the
register may not always
be removed.

I will submit a patch which is to fix the 3 component vector load store bug.
And with that patch,
you can reproduce the above issue easily.

Although we can still workaround this bug by ignore one instruction when we
found its value
is not exists. I don't think that's a good idea. As that may hide some real
bug, and will decrease
our code's readability.

Just as we discussed at the first time we found this issue, is it possible
to make scalarize
aware of those value deleted in other pass, and thus keep scalarize
consistent with other passes.

> -----Original Message-----
> From: beignet-bounces+zhigang.gong=linux.intel.com at lists.freedesktop.org
>
[mailto:beignet-bounces+zhigang.gong=linux.intel.com at lists.freedesktop.org]
> On Behalf Of Yang Rong
> Sent: Thursday, May 30, 2013 1:55 PM
> To: beignet at lists.freedesktop.org
> Cc: Yang Rong
> Subject: [Beignet] [PATCH] Fix a random assert in scalarize pass.
> 
> There is a random assert in unit.newValueProxy call from scalarzie pass.
> Because pass excute order is Scalarize->DeadInstElimination->Scalarzie->
> DeadInstElimination->GenPass->GenPass.
> The previous fake value, created in first Scalarzie pass maybe deleted,
and new
> fake address have same address in function unit.newValueProxy.
> Because GenPass haven't excuted, can't clear the map.
> Remove assert for simple because it is not a error.
> 
> Also fix a typo.
> 
> Signed-off-by: Yang Rong <rong.r.yang at intel.com>
> ---
>  backend/src/ir/unit.hpp             |    4 +++-
>  backend/src/llvm/llvm_scalarize.cpp |   10 +++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/backend/src/ir/unit.hpp b/backend/src/ir/unit.hpp index
> 3b293f5..77cc3b4 100644
> --- a/backend/src/ir/unit.hpp
> +++ b/backend/src/ir/unit.hpp
> @@ -85,7 +85,9 @@ namespace ir {
>                         uint32_t fakeIndex = 0u) {
>        const ValueIndex key(fake, fakeIndex);
>        const ValueIndex value(real, realIndex);
> -      GBE_ASSERT(valueMap.find(key) == valueMap.end()); // Do not insert
> twice
> +      //Previous fake value maybe deleted and new fake address have same
> address
> +      //Remove assert because it is not a error case.
> +      //GBE_ASSERT(valueMap.find(key) == valueMap.end());
>        valueMap[key] = value;
>      }
>      /*! Return the value map */
> diff --git a/backend/src/llvm/llvm_scalarize.cpp
> b/backend/src/llvm/llvm_scalarize.cpp
> index bc66549..9b07413 100644
> --- a/backend/src/llvm/llvm_scalarize.cpp
> +++ b/backend/src/llvm/llvm_scalarize.cpp
> @@ -182,7 +182,7 @@ namespace gbe {
>      bool IsPerComponentOp(const Value* value);
> 
>      //these function used to add extract and insert instructions when
> load/store etc.
> -    void extractFromeVector(Value* insn);
> +    void extractFromVector(Value* insn);
>      Value* InsertToVector(Value* insn, Value* vecValue);
> 
>      Type* GetBasicType(Value* value) {
> @@ -581,7 +581,7 @@ namespace gbe {
>      return true;
>    }
> 
> -  void Scalarize::extractFromeVector(Value* insn) {
> +  void Scalarize::extractFromVector(Value* insn) {
>      VectorValues& vVals = vectorVals[insn];
> 
>      for (int i = 0; i < GetComponentCount(insn); ++i) { @@ -645,7 +645,7
@@
> namespace gbe {
>            case GEN_OCL_GET_IMAGE_WIDTH:
>            case GEN_OCL_GET_IMAGE_HEIGHT:
>            {
> -            extractFromeVector(call);
> +            extractFromVector(call);
>              break;
>            }
>            case GEN_OCL_WRITE_IMAGE10:
> @@ -673,7 +673,7 @@ namespace gbe {
> 
>    bool Scalarize::scalarizeLoad(LoadInst* ld)
>    {
> -    extractFromeVector(ld);
> +    extractFromVector(ld);
>      return false;
>    }
> 
> @@ -738,7 +738,7 @@ namespace gbe {
>        Type *type = I->getType();
> 
>        if(type->isVectorTy())
> -        extractFromeVector(I);
> +        extractFromVector(I);
>      }
>      return;
>    }
> --
> 1.7.9.5
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet