Adds handling for llvm.memmove-intrinsic #209
Conversation
|
You can add SPIRVFunction::addVariable to make sure the variable instruction is properly ordered, then modify SPIRVModuleImpl::addVariable to call it. |
|
So, this version does everything correct, as far as I can tell and is ready to be pulled. |
lib/SPIRV/libSPIRV/SPIRVFunction.cpp
Outdated
| SPIRVVariable *SPIRVFunction::addVariable(SPIRVVariable *Var) | ||
| { | ||
| //according to the SPIR-V specs, the Variable (with Storage Class Function) need to be at the beginning of the first block of the function, see section 2.4 (Logical Layout of a Module) | ||
| return BBVec.at(0)->addVariable(Var); |
There was a problem hiding this comment.
needs to check if BBVec is empty. If so add a BB.
| assert(Var && "Invalid variable"); | ||
| Module->add(Var); | ||
| Var->setParent(this); | ||
| InstVec.insert(InstVec.begin(), Var); |
There was a problem hiding this comment.
This will cause auto vars in source code emitted in reverse order and break existing lit tests.
Should find the first non-Variable instruction and insert before that.
|
Please check the Travis CI build status. It should pass. |
|
Made the requested changes. Also now the tests compile. I'm not quite sure though about adding the new BasicBlock in SPIRVFunction::addVariable (line 70). Do I have to specify an ID? If so, where do I get it from? |
| Var->setParent(this); | ||
| auto It = InstVec.begin(); | ||
| while(It != InstVec.end()) | ||
| { |
There was a problem hiding this comment.
Minor style comment.
Please, follow the existing style - move { to the line above.
I think the whole loop can be replaced with standard find algorithm.
lib/SPIRV/libSPIRV/SPIRVFunction.cpp
Outdated
| @@ -67,6 +67,16 @@ SPIRVFunctionParameter::foreachAttr( | |||
| } | |||
| } | |||
|
|
|||
| SPIRVVariable *SPIRVFunction::addVariable(SPIRVVariable *Var) | |||
| { | |||
There was a problem hiding this comment.
Please, move the brace to the line above.
lib/SPIRV/libSPIRV/SPIRVFunction.cpp
Outdated
| { | ||
| //according to the SPIR-V specs, the Variable (with Storage Class Function) need to be at the beginning of the first block of the function, see section 2.4 (Logical Layout of a Module) | ||
| if(getNumBasicBlock() == 0) | ||
| { |
There was a problem hiding this comment.
Please, move the brace to the line above.
|
Applied the style-changes and added a test case. Is this test-case any good? If not, how should I write it? |
test/SPIRV/llvm.memmove.ll
Outdated
|
|
||
| ; CHECK-NOT: llvm.memmove | ||
|
|
||
| ; CHECK: Variable [[voidPtr:[0-9]+]] [[mem:[0-9]+]] 7 |
There was a problem hiding this comment.
There is no need in voidPtr variable - it's not used in the test. Suggest replacing with {{[0-9]+}}.
|
The tests in DEBUG mode on travis CI fail with "Object type must be an integer type scalar" in SPIRVInstruction.h:1882. I don't understand, why the pointer-type needs to be an integer. Especially, because the next line seems to accept the |
|
It looks like a bug in the assertion statement. Here is the quote from the spec: Size must be 0 if Pointer is a pointer to a non-void type or the Addresses capability is not being used. If Size is non-zero, it is the number of bytes of memory whose lifetime is starting. Its type must be an integer type scalar. It is treated as unsigned; if its type has Signedness of 1, its sign bit cannot be set." "Size" parameter type must be integer. "Pointer" can point to non-integer type values. Could you fix the assertion statement, please? |
| @@ -1877,7 +1877,6 @@ class SPIRVLifetime : public SPIRVInstruction { | |||
| assert(Obj->getStorageClass() == StorageClassFunction && | |||
| "Invalid storage class"); | |||
| assert(Obj->getType()->isTypePointer() && | |||
| Obj->getType()->getPointerElementType()->isTypeInt() && | |||
| "Object type must be an integer type scalar"); | |||
There was a problem hiding this comment.
Please, update the message to avoid confusion.
"Objects type must be a pointer".
lib/SPIRV/libSPIRV/SPIRVFunction.cpp
Outdated
| @@ -67,6 +67,14 @@ SPIRVFunctionParameter::foreachAttr( | |||
| } | |||
| } | |||
|
|
|||
| SPIRVVariable *SPIRVFunction::addVariable(SPIRVVariable *Var) { | |||
| //according to the SPIR-V specs, the Variable (with Storage Class Function) need to be at the beginning of the first block of the function, see section 2.4 (Logical Layout of a Module) | |||
| if(getNumBasicBlock() == 0) { | |||
|
There is still travis-ci build failure. Looking at the result: FAIL: LLVM :: SPIRV/llvm.memmove.ll (8805 of 12438)
|
|
I can't figure out, why this test fails. So, besides removing the test I don't know of any fix. |
lib/SPIRV/SPIRVWriter.cpp
Outdated
| @@ -1303,6 +1303,28 @@ LLVMToSPIRV::transIntrinsicInst(IntrinsicInst *II, SPIRVBasicBlock *BB) { | |||
| transValue(II->getOperand(2), BB), | |||
| getMemoryAccess(cast<MemIntrinsic>(II)), | |||
| BB); | |||
| case Intrinsic::memmove: { | |||
| //Since memory areas for source and dest can overlap, 1. copy to temporary, 2. copy to destination | |||
| SPIRVType *Ty = transType(II->getType()); | |||
There was a problem hiding this comment.
should be II->getOperand(0)->getType()
test/SPIRV/llvm.memmove.ll
Outdated
| @@ -0,0 +1,50 @@ | |||
| ; RUN: llvm-as %s -o %t.bc | |||
There was a problem hiding this comment.
can we make it a transcodeing test to make sure the generated SPIRV can be translated back to LLVM IR?
|
I think I know why. The type passed to addVariable is wrong. I've added inline comments about that. |
|
Also, can you add an assertion to SPIRVVariable::validate to assert the type is pointer type? Thanks. |
|
Applied the requested changes, the trancoding-test fails, since the method-signatures do not match for the With the single function declaration for As far as i can tell, there should be 2 declarations, something like: |
|
It seems to be more complicated than expected. One issue is the type of the temporary variable. It should be either a pointer to the object type to be moved, or a pointer to an array of chars which have the same size of the memmov operation. Another issue is that adding an existing type to SPIR-V module may cause problem. The SPIR-V module does not have a fold set to guarantee uniqueness of types, so the user has to make sure no duplicate types are added to SPIR-V module. For this type of transformation, it may be easier to write a pass like SPIRVLowerBool to transform memmov to memcpy in LLVM IR. |
|
Using -O3 we transform two memcpy calls to single memmove call. For translation to SPIR-V purposes it looks reasonable to transform the memmove back to memcpy. Maybe we just should not use -O3 in the first place? |
|
That can be a workaround if users are using Clang. I think it is better to be able to lower memmov. |
|
I need the other optimizations run with |
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| auto Src = I.getRawSource(); | ||
| auto SrcTy = Src->getType(); | ||
| ConstantInt *Length = nullptr; | ||
| if(isa<ConstantInt>(I.getLength())) //the length (in bytes) could be non-constant |
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| virtual void visitMemMoveInst(MemMoveInst &I) { | ||
| IRBuilder<> Builder(I.getParent()); | ||
| Builder.SetInsertPoint(&I); | ||
| auto Dest = I.getRawDest(); |
There was a problem hiding this comment.
For pointers better use auto *. Same as below.
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| auto SrcTy = Src->getType(); | ||
| ConstantInt *Length = nullptr; | ||
| if(isa<ConstantInt>(I.getLength())) //the length (in bytes) could be non-constant | ||
| Length = ConstantInt::get(Type::getInt64Ty(*Context), cast<ConstantInt>(I.getLength())->getZExtValue()); |
There was a problem hiding this comment.
Can you just use I.getLength() instead of create a new ConstantInt?
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| auto Align = I.getAlignment(); | ||
| auto Volatile = I.isVolatile(); | ||
|
|
||
| auto Alloca = Builder.CreateAlloca(SrcTy->getPointerElementType(), SrcTy->isArrayTy() ? Builder.getInt32(SrcTy->getArrayNumElements()) : nullptr); |
There was a problem hiding this comment.
this line may exceed 80 columns.
Please follow http://llvm.org/docs/CodingStandards.html
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| auto Src = I.getRawSource(); | ||
| auto SrcTy = Src->getType(); | ||
| ConstantInt *Length = nullptr; | ||
| if(isa<ConstantInt>(I.getLength())) //the length (in bytes) could be non-constant |
There was a problem hiding this comment.
If it is not constant we can create a loop and each time copy one chunk of fixed length.
However, for now, maybe just report_fatal_error as "llvm.memmov of non-constant length not supported".
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| Length = ConstantInt::get(Type::getInt64Ty(*Context), cast<ConstantInt>(I.getLength())->getZExtValue()); | ||
| if(isa<BitCastInst>(Src)) //the source could be bit-cast from another type, need the original type for the allocation | ||
| SrcTy = cast<BitCastInst>(Src)->getOperand(0)->getType(); | ||
| auto Align = I.getAlignment(); |
There was a problem hiding this comment.
need to check the size of the variable matches the length of memmov. If not report_fatal_error.
lib/SPIRV/SPIRVLowerMemmove.cpp
Outdated
| @@ -105,6 +125,7 @@ class SPIRVLowerMemmove: public ModulePass, | |||
| static char ID; | |||
| private: | |||
| LLVMContext *Context; | |||
| Module *Mod; | |||
There was a problem hiding this comment.
line 90 to query the size of the allocated type: Mod->getDataLayout()->getTypeSizeInBits(SrcTy->getPointerElementType()). I created the member, since I didn't know how else to get the Module instance.
There was a problem hiding this comment.
Sorry I missed that. Then it looks fine.
|
There are too many commits in this pull request. Can you squash them into one commit? Otherwise LGTM. |
doe300
force-pushed
the
khronos/spirv-3.6.1
branch
from
c549c2b to
d60f924
Compare
Closes #205.
Produces following SPIR-V output for the example given in #205:
This result is "correct" in the sense, that the code does the right thing, but:
voidand has no size specified.LifetimeStartandLifeTimeStopbefore/after theCopyMemorySizedinstruction.