[Beignet] [PATCH v2] GBE: fix a corner case which refer to an undefined value.
Zhigang Gong
zhigang.gong at linux.intel.com
Thu Dec 19 17:36:21 PST 2013
Please ignore this version too. I just sent out another patch to
fix this problem better.
On Wed, Dec 18, 2013 at 02:47:59PM +0800, Zhigang Gong wrote:
> Clang/llvm may generate some code similar to the following IRs:
>
> ... (there is no definition of %7)
> br label 2
>
> label1:
> %10 = add %7, %6
> ...
> ret
>
> label2:
> %7 = ...
> br label1
>
> The value %7 is assigned after label2 but is referred at label1.
> From the control flow, the IRs is valid. As the reference will
> be executed after the assignment.
>
> But our current virtual register allocation assume that all the
> values are assigned before any usage from the instructions' order
> view which is incorrect for this case.
>
> This patch choose a simple way to fix this issue. It allocate
> register when it fails to get one. And latter when emit the assignment
> instruction, it just ignore the duplicate allocation.
>
> v2: handle the case when %7 is a proxyValue.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
> ---
> backend/src/llvm/llvm_gen_backend.cpp | 77 ++++++++++++++++++++++++++++-----
> 1 file changed, 65 insertions(+), 12 deletions(-)
>
> diff --git a/backend/src/llvm/llvm_gen_backend.cpp b/backend/src/llvm/llvm_gen_backend.cpp
> index 0d4fb87..2104a36 100644
> --- a/backend/src/llvm/llvm_gen_backend.cpp
> +++ b/backend/src/llvm/llvm_gen_backend.cpp
> @@ -302,6 +302,8 @@ namespace gbe
> void clear(void) {
> valueMap.clear();
> scalarMap.clear();
> + incomplRegMap.clear();
> + needPatchIncomplReg = false;
> }
> /*! Some values will not be allocated. For example, a bit-cast destination
> * like: %fake = bitcast %real or a vector insertion since we do not have
> @@ -315,6 +317,9 @@ namespace gbe
> const ValueIndex value(real, realIndex);
> GBE_ASSERT(valueMap.find(key) == valueMap.end()); // Do not insert twice
> valueMap[key] = value;
> +
> + if (scalarMap.find(std::make_pair(fake, fakeIndex)) != scalarMap.end())
> + needPatchIncomplReg = true;
> }
> /*! Mostly used for the preallocated registers (lids, gids) */
> void newScalarProxy(ir::Register reg, Value *value, uint32_t index = 0u) {
> @@ -323,7 +328,7 @@ namespace gbe
> scalarMap[key] = reg;
> }
> /*! Allocate a new scalar register */
> - ir::Register newScalar(Value *value, Value *key = NULL, uint32_t index = 0u)
> + ir::Register newScalar(Value *value, Value *key = NULL, uint32_t index = 0u, bool incomplete = false)
> {
> // we don't allow normal constant, but GlobalValue is a special case,
> // it needs a register to store its address
> @@ -336,7 +341,7 @@ namespace gbe
> case Type::DoubleTyID:
> case Type::PointerTyID:
> GBE_ASSERT(index == 0);
> - return this->newScalar(value, key, type, index);
> + return this->newScalar(value, key, type, index, incomplete);
> break;
> case Type::VectorTyID:
> {
> @@ -347,7 +352,7 @@ namespace gbe
> elementTypeID != Type::FloatTyID &&
> elementTypeID != Type::DoubleTyID)
> GBE_ASSERTM(false, "Vectors of elements are not supported");
> - return this->newScalar(value, key, elementType, index);
> + return this->newScalar(value, key, elementType, index, incomplete);
> break;
> }
> default: NOT_SUPPORTED;
> @@ -401,15 +406,36 @@ namespace gbe
> CPV = extractConstantElem(CPV, index);
> return (CPV && (isa<UndefValue>(CPV)));
> }
> +
> + void patchIncomplReg(ir::Function &fn);
> private:
> + /*! Map value pair to ir::register for the incomplete register patch usage */
> + /* These register are referred before their assignment physically, */
> + /* we may allocate the registers at the reference time, but latter, if the
> + value is just a proxy value, we have to patch all the previous allocated
> + registers to the real registers pointed by the proxy value. */
> + struct ValuePair {
> + ValuePair(Value *_v, uint32_t _id) : v(_v), elemID(_id) { };
> + Value *v;
> + uint32_t elemID;
> + };
> + map<ir::Register, struct ValuePair> incomplRegMap;
> + bool needPatchIncomplReg;
> +
> /*! This creates a scalar register for a Value (index is the vector index when
> * the value is a vector of scalars)
> */
> - ir::Register newScalar(Value *value, Value *key, Type *type, uint32_t index) {
> + ir::Register newScalar(Value *value, Value *key, Type *type, uint32_t index, bool incomplete) {
> const ir::RegisterFamily family = getFamily(ctx, type);
> - const ir::Register reg = ctx.reg(family);
> key = key == NULL ? value : key;
> + if (scalarMap.find(std::make_pair(key, index)) != scalarMap.end())
> + return scalarMap[std::make_pair(key, index)];
> + const ir::Register reg = ctx.reg(family);
> this->insertRegister(reg, key, index);
> + if (incomplete) {
> + struct ValuePair vp(value, index);
> + incomplRegMap.insert(std::make_pair(reg, vp));
> + }
> return reg;
> }
> /*! Map value to ir::Register */
> @@ -500,7 +526,7 @@ namespace gbe
> /*! Each block end may require to emit MOVs for further PHIs */
> void emitMovForPHI(BasicBlock *curr, BasicBlock *succ);
> /*! Alocate one or several registers (if vector) for the value */
> - INLINE void newRegister(Value *value, Value *key = NULL);
> + INLINE void newRegister(Value *value, Value *key = NULL, bool incomplete = false);
> /*! get the register for a llvm::Constant */
> ir::Register getConstantRegister(Constant *c, uint32_t index = 0);
> /*! Return a valid register from an operand (can use LOADI to make one) */
> @@ -853,7 +879,7 @@ namespace gbe
> return processConstant<ir::ImmediateIndex>(CPV, NewImmediateFunctor(ctx), index);
> }
>
> - void GenWriter::newRegister(Value *value, Value *key) {
> + void GenWriter::newRegister(Value *value, Value *key, bool incomplete) {
> auto type = value->getType();
> auto typeID = type->getTypeID();
> switch (typeID) {
> @@ -861,14 +887,15 @@ namespace gbe
> case Type::FloatTyID:
> case Type::DoubleTyID:
> case Type::PointerTyID:
> - regTranslator.newScalar(value, key);
> + regTranslator.newScalar(value, key, 0, incomplete);
> break;
> case Type::VectorTyID:
> {
> auto vectorType = cast<VectorType>(type);
> const uint32_t elemNum = vectorType->getNumElements();
> - for (uint32_t elemID = 0; elemID < elemNum; ++elemID)
> - regTranslator.newScalar(value, key, elemID);
> + for (uint32_t elemID = 0; elemID < elemNum; ++elemID) {
> + regTranslator.newScalar(value, key, elemID, incomplete);
> + }
> break;
> }
> default: NOT_SUPPORTED;
> @@ -971,8 +998,11 @@ namespace gbe
> if(isa<Constant>(value)) {
> Constant *c = dyn_cast<Constant>(value);
> return getConstantRegister(c, elemID);
> - } else
> + } else {
> + if (!(regTranslator.valueExists(value, elemID)))
> + this->newRegister(value, NULL, true);
> return regTranslator.getScalar(value, elemID);
> + }
> }
>
> INLINE Value *GenWriter::getPHICopy(Value *PHI) {
> @@ -1293,6 +1323,28 @@ namespace gbe
> });
> }
>
> + void RegisterTranslator::patchIncomplReg(ir::Function &fn)
> + {
> + // Traverse all blocks and patch the incomplete registers.
> + if (!needPatchIncomplReg) return;
> + fn.foreachBlock([&](ir::BasicBlock &bb)
> + {
> + // Top bottom traversal patch incomplete registers.
> + bb.foreach([&](ir::Instruction &insn)
> + {
> + const uint32_t srcNum = insn.getSrcNum();
> + for (uint32_t srcID = 0; srcID < srcNum; ++srcID) {
> + const ir::Register src = insn.getSrc(srcID);
> + const auto &vp = incomplRegMap.find(src);
> + if (vp == incomplRegMap.end())
> + continue;
> + insn.setSrc(srcID, this->getScalar(vp->second.v, vp->second.elemID));
> + }
> + });
> + });
> + }
> +
> +
> void GenWriter::removeLOADIs(const ir::Liveness &liveness, ir::Function &fn)
> {
> // We store the last write and last read for each register
> @@ -1465,7 +1517,8 @@ namespace gbe
>
> // Liveness can be shared when we optimized the immediates and the MOVs
> const ir::Liveness liveness(fn);
> -
> + // Patch the incomplete registers.
> + regTranslator.patchIncomplReg(fn);
> if (OCL_OPTIMIZE_LOADI) this->removeLOADIs(liveness, fn);
> if (OCL_OPTIMIZE_PHI_MOVES) this->removeMOVs(liveness, fn);
> }
> --
> 1.7.9.5
>
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
More information about the Beignet
mailing list