[Beignet] [PATCH] GBE: Clear the value map when start a new scalarize pass.

[He Junyan]

test your patch, and not found regression for Unit Test and Piglit


On 07/04/2013 07:21 PM, Zhigang Gong wrote:
> The scalarize pass is a function pass, and the valueMap should
> be a per-function data rather than a per-unit data. The reason
> we put it in the unit data structure is that the scalarize pass
> is before the GenWriter pass thus there is no ir::Function exists.
>
> As there may be multiple kernel functions in one unit, if we don't
> clear the valueMap each time running a new scalarize pass, the previous
> data may cause some unexpected behaviour. For example, the previous
> instructions have been already erased, then latter a new instruction
> in this function may be created in the same position of the erased
> instruction, then it breaks this valueMap. That's the root cause why
> we run the unit test several times and may encounter an assertion
> sometime.
>
> This commit also modify the ir::unit layer implementation to remove
> the dependency of llvm from that layer. In general, we should not add
> llvm related code to the ir layer.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at linux.intel.com>
> ---
>   backend/src/ir/unit.cpp               |   14 --------------
>   backend/src/ir/unit.hpp               |   20 +++++++-------------
>   backend/src/llvm/llvm_gen_backend.cpp |    7 ++++---
>   backend/src/llvm/llvm_scalarize.cpp   |    2 +-
>   4 files changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/backend/src/ir/unit.cpp b/backend/src/ir/unit.cpp
> index 01e1eb1..4aeffe9 100644
> --- a/backend/src/ir/unit.cpp
> +++ b/backend/src/ir/unit.cpp
> @@ -21,12 +21,6 @@
>    * \file unit.cpp
>    * \author Benjamin Segovia <benjamin.segovia at intel.com>
>    */
> -#include "llvm/Config/config.h"
> -#if LLVM_VERSION_MINOR <= 2
> -#include "llvm/Instructions.h"
> -#else
> -#include "llvm/IR/Instructions.h"
> -#endif  /* LLVM_VERSION_MINOR <= 2 */
>   #include "ir/unit.hpp"
>   #include "ir/function.hpp"
>   
> @@ -59,14 +53,6 @@ namespace ir {
>       constantSet.append(data, name, size, alignment);
>     }
>   
> -  void Unit::removeDeadValues()
> -  {
> -    for(auto &it : valueMap) {
> -      llvm::Instruction* I = llvm::dyn_cast<llvm::Instruction>(it.first.first);  //fake value
> -      if((I == NULL) || (I->getParent() == NULL))
> -        valueMap.erase(it.first);
> -    }
> -  }
>     std::ostream &operator<< (std::ostream &out, const Unit &unit) {
>       unit.apply([&out] (const Function &fn) { out << fn << std::endl; });
>       return out;
> diff --git a/backend/src/ir/unit.hpp b/backend/src/ir/unit.hpp
> index 1017f5f..9e3d66a 100644
> --- a/backend/src/ir/unit.hpp
> +++ b/backend/src/ir/unit.hpp
> @@ -24,13 +24,6 @@
>   #ifndef __GBE_IR_UNIT_HPP__
>   #define __GBE_IR_UNIT_HPP__
>   
> -#include "llvm/Config/config.h"
> -#if LLVM_VERSION_MINOR <= 2
> -#include "llvm/Value.h"
> -#else
> -#include "llvm/IR/Value.h"
> -#endif  /* LLVM_VERSION_MINOR <= 2 */
> -
>   #include "ir/constant.hpp"
>   #include "ir/register.hpp"
>   #include "sys/hash_map.hpp"
> @@ -49,7 +42,7 @@ namespace ir {
>     {
>     public:
>       typedef hash_map<std::string, Function*> FunctionSet;
> -    typedef std::pair<llvm::Value*, uint32_t> ValueIndex;
> +    typedef std::pair<void*, uint32_t> ValueIndex;
>       /*! Create an empty unit */
>       Unit(PointerSize pointerSize = POINTER_32_BITS);
>       /*! Release everything (*including* the function pointers) */
> @@ -84,8 +77,8 @@ namespace ir {
>       /*! Some values will not be allocated. For example a vector extract and
>        * a vector insertion when scalarize the vector load/store
>        */
> -    void newValueProxy(llvm::Value *real,
> -                       llvm::Value *fake,
> +    void newValueProxy(void *real,
> +                       void *fake,
>                          uint32_t realIndex = 0u,
>                          uint32_t fakeIndex = 0u) {
>         const ValueIndex key(fake, fakeIndex);
> @@ -93,10 +86,11 @@ namespace ir {
>         GBE_ASSERT(valueMap.find(key) == valueMap.end()); // Do not insert twice
>         valueMap[key] = value;
>       }
> -    /* remove fake values that removed by other pass */
> -    void removeDeadValues(void);
> +
> +    void clearValueMap() { valueMap.clear(); }
> +
>       /*! Return the value map */
> -    const map<ValueIndex, ValueIndex>& getValueMap(void) const { return valueMap; }
> +    const map<ValueIndex, ValueIndex> &getValueMap(void) const { return valueMap; }
>     private:
>       friend class ContextInterface; //!< Can free modify the unit
>       hash_map<std::string, Function*> functions; //!< All the defined functions
> diff --git a/backend/src/llvm/llvm_gen_backend.cpp b/backend/src/llvm/llvm_gen_backend.cpp
> index 40f2667..36d8129 100644
> --- a/backend/src/llvm/llvm_gen_backend.cpp
> +++ b/backend/src/llvm/llvm_gen_backend.cpp
> @@ -307,8 +307,10 @@ namespace gbe
>       }
>       /*! After scalarize pass, there are some valueMap in unit,
>        *  use this function to copy from unit valueMap */
> -    void initValueMap(const map<ValueIndex, ValueIndex>& vMap) {
> -      valueMap.insert(vMap.begin(), vMap.end());
> +    void initValueMap(const map<ir::Unit::ValueIndex, ir::Unit::ValueIndex> &vMap) {
> +      for(auto &it : vMap)
> +        newValueProxy((Value*)it.second.first, (Value*)it.first.first,
> +                      it.second.second, it.first.second);
>       }
>       /*! Mostly used for the preallocated registers (lids, gids) */
>       void newScalarProxy(ir::Register reg, Value *value, uint32_t index = 0u) {
> @@ -1207,7 +1209,6 @@ namespace gbe
>       }
>   
>       ctx.startFunction(F.getName());
> -    unit.removeDeadValues();
>       this->regTranslator.clear();
>       this->regTranslator.initValueMap(unit.getValueMap());
>       this->labelMap.clear();
> diff --git a/backend/src/llvm/llvm_scalarize.cpp b/backend/src/llvm/llvm_scalarize.cpp
> index bab2236..41674b6 100644
> --- a/backend/src/llvm/llvm_scalarize.cpp
> +++ b/backend/src/llvm/llvm_scalarize.cpp
> @@ -773,7 +773,7 @@ namespace gbe {
>       intTy = IntegerType::get(module->getContext(), 32);
>       floatTy = Type::getFloatTy(module->getContext());
>       builder = new IRBuilder<>(module->getContext());
> -    unit.removeDeadValues();
> +    unit.clearValueMap();
>   
>       scalarizeArgs(F);
>       typedef ReversePostOrderTraversal<Function*> RPOTType;

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/beignet/attachments/20130705/499d663e/attachment-0001.html>