search

KhronosGroup / SPIRV-LLVM

This project is no longer active. Please join us at
https://github.com/KhronosGroup/SPIRV-LLVM-Translator
Other
262 stars 60 forks source link

Adds handling for llvm.memmove-intrinsic #209

Closed doe300 closed 7 years ago

doe300 commented 7 years ago

Closes #205.

Produces following SPIR-V output for the example given in #205:

119734787 65536 393230 22 0 
2 Capability Addresses 
2 Capability Kernel 
2 Capability Vector16 
2 Capability Int8 
5 ExtInstImport 1 "OpenCL.std"
3 MemoryModel 1 2 
6 EntryPoint 6 12 "test_struct"
3 Source 3 102000 
7 Name 3 "struct.SomeStruct"
3 Name 13 "in"
3 Name 14 "out"
4 Decorate 21 FuncParamAttr 5 
2 DecorationGroup 21 
4 Decorate 13 FuncParamAttr 6 
4 GroupDecorate 21 13 14 
4 TypeInt 6 32 0 
4 TypeInt 8 8 0 
4 Constant 6 7 60 
4 Constant 6 20 128 
2 TypeVoid 2 
3 TypeFloat 4 32 
4 TypeVector 5 4 16 
4 TypeArray 9 8 7 
5 TypeStruct 3 5 6 9 
4 TypePointer 10 5 3 
5 TypeFunction 11 2 10 10 
4 TypePointer 16 5 8 

5 Function 2 12 0 11 
3 FunctionParameter 10 13 
3 FunctionParameter 10 14 

2 Label 15 
4 Variable 2 19 7 
4 Bitcast 16 17 13 
4 Bitcast 16 18 14 
3 LifetimeStart 19 128 
6 CopyMemorySized 19 17 20 2 64 
6 CopyMemorySized 18 19 20 2 64 
3 LifetimeStop 19 128 
1 Return 

1 FunctionEnd

This result is "correct" in the sense, that the code does the right thing, but:

yxsamliu commented 7 years ago

You can add SPIRVFunction::addVariable to make sure the variable instruction is properly ordered, then modify SPIRVModuleImpl::addVariable to call it.

doe300 commented 7 years ago

So, this version does everything correct, as far as I can tell and is ready to be pulled.

yxsamliu commented 7 years ago

Please check the Travis CI build status. It should pass.

doe300 commented 7 years ago

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?

doe300 commented 7 years ago

Applied the style-changes and added a test case.

Is this test-case any good? If not, how should I write it?

doe300 commented 7 years ago

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 void* type.

bader commented 7 years ago

It looks like a bug in the assertion statement.

Here is the quote from the spec: "Pointer is a pointer to the object whose lifetime is starting. Its type must be an OpTypePointer with Storage Class Function.

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?

yxsamliu commented 7 years ago

There is still travis-ci build failure. Looking at the result:

FAIL: LLVM :: SPIRV/llvm.memmove.ll (8805 of 12438) **** TEST 'LLVM :: SPIRV/llvm.memmove.ll' FAILED **** Script:

/home/travis/build/KhronosGroup/SPIRV-LLVM/build/./bin/llvm-as /home/travis/build/KhronosGroup/SPIRV-LLVM/test/SPIRV/llvm.memmove.ll -o /home/travis/build/KhronosGroup/SPIRV-LLVM/build/test/SPIRV/Output/llvm.memmove.ll.tmp.bc /home/travis/build/KhronosGroup/SPIRV-LLVM/build/./bin/llvm-spirv /home/travis/build/KhronosGroup/SPIRV-LLVM/build/test/SPIRV/Output/llvm.memmove.ll.tmp.bc -spirv-text -o - | /home/travis/build/KhronosGroup/SPIRV-LLVM/build/./bin/FileCheck /home/travis/build/KhronosGroup/SPIRV-LLVM/test/SPIRV/llvm.memmove.ll

Exit Code: 2

Command Output (stderr):

llvm-spirv: /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/libSPIRV/SPIRVInstruction.h:1880: virtual void SPIRV::SPIRVLifetime<256>::validate() const [OC = 256]: Assertion `Obj->getType()->isTypePointer() && "Objects type must be a pointer"' failed. Stack dump:

  1. Program arguments: /home/travis/build/KhronosGroup/SPIRV-LLVM/build/./bin/llvm-spirv /home/travis/build/KhronosGroup/SPIRV-LLVM/build/test/SPIRV/Output/llvm.memmove.ll.tmp.bc -spirv-text -o -
  2. Running pass 'LLVMToSPIRV' on module '/home/travis/build/KhronosGroup/SPIRV-LLVM/build/test/SPIRV/Output/llvm.memmove.ll.tmp.bc'.

    0 0x80b21e llvm::sys::PrintStackTrace(_IO_FILE*) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/Support/Unix/Signals.inc:422:15

    1 0x80bfdb PrintStackTraceSignalHandler(void*) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/Support/Unix/Signals.inc:481:1

    2 0x80e704 SignalHandler(int) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/Support/Unix/Signals.inc:198:60

    3 0x2adbb96de330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)

    4 0x2adbba35bc37 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x36c37)

    5 0x2adbba35f028 abort (/lib/x86_64-linux-gnu/libc.so.6+0x3a028)

    6 0x2adbba354bf6 (/lib/x86_64-linux-gnu/libc.so.6+0x2fbf6)

    7 0x2adbba354ca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)

    8 0x4a85f6 SPIRV::SPIRVLifetime<(spv::Op)256>::validate() const /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/libSPIRV/SPIRVInstruction.h:1881:10

    9 0x498c4d SPIRV::SPIRVLifetime<(spv::Op)256>::SPIRVLifetime(unsigned int, unsigned int, SPIRV::SPIRVBasicBlock*) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/libSPIRV/SPIRVInstruction.h:1861:5

    10 0x498b6d SPIRV::SPIRVModuleImpl::addLifetimeInst(spv::Op, SPIRV::SPIRVValue, unsigned int, SPIRV::SPIRVBasicBlock) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/libSPIRV/SPIRVModule.cpp:1125:12

    11 0x5919d9 SPIRV::LLVMToSPIRV::transIntrinsicInst(llvm::IntrinsicInst, SPIRV::SPIRVBasicBlock) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:1313:5

    12 0x58f1b7 SPIRV::LLVMToSPIRV::transValueWithoutDecoration(llvm::Value, SPIRV::SPIRVBasicBlock, bool) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:1157:22

    13 0x58a1b3 SPIRV::LLVMToSPIRV::transValue(llvm::Value, SPIRV::SPIRVBasicBlock, bool) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:812:13

    14 0x5938b2 SPIRV::LLVMToSPIRV::transFunction(llvm::Function*) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:1486:7

    15 0x593d8b SPIRV::LLVMToSPIRV::translate() /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:1530:5

    16 0x5a047f SPIRV::LLVMToSPIRV::runOnModule(llvm::Module&) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:178:5

    17 0x73da7c (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/IR/LegacyPassManager.cpp:1616:23

    18 0x73d65e llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/IR/LegacyPassManager.cpp:1723:16

    19 0x73e041 llvm::legacy::PassManager::run(llvm::Module&) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/IR/LegacyPassManager.cpp:1756:10

    20 0x5967d1 llvm::WriteSPIRV(llvm::Module*, llvm::raw_ostream&, std::string&) /home/travis/build/KhronosGroup/SPIRV-LLVM/lib/SPIRV/SPIRVWriter.cpp:1857:3

    21 0x409828 convertLLVMToSPIRV() /home/travis/build/KhronosGroup/SPIRV-LLVM/tools/llvm-spirv/llvm-spirv.cpp:154:8

    22 0x408fd8 main /home/travis/build/KhronosGroup/SPIRV-LLVM/tools/llvm-spirv/llvm-spirv.cpp:312:12

    23 0x2adbba346f45 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f45)

    24 0x4088e4 _start (/home/travis/build/KhronosGroup/SPIRV-LLVM/build/bin/llvm-spirv+0x4088e4)

Can you fix that?

doe300 commented 7 years ago

I can't figure out, why this test fails. So, besides removing the test I don't know of any fix.

yxsamliu commented 7 years ago

I think I know why. The type passed to addVariable is wrong. I've added inline comments about that.

yxsamliu commented 7 years ago

Also, can you add an assertion to SPIRVVariable::validate to assert the type is pointer type? Thanks.

doe300 commented 7 years ago

Applied the requested changes, the trancoding-test fails, since the method-signatures do not match for the llvm.memcpy calls:

Fails to verify module: Call parameter type does not match function signature!
  %1 = alloca i8
 i8 addrspace(1)*  call void @llvm.memcpy.p1i8.p1i8.i32(i8* %1, i8 addrspace(1)* %2, i32 128, i32 64, i1 false)
Call parameter type does not match function signature!
  %1 = alloca i8
 i8 addrspace(1)*  call void @llvm.memcpy.p1i8.p1i8.i32(i8 addrspace(1)* %3, i8* %1, i32 128, i32 64, i1 false)

With the single function declaration for llvm.memcpy:

declare void @llvm.memcpy.p1i8.p1i8.i32(i8 addrspace(1)* nocapture, i8 addrspace(1)* nocapture readonly, i32, i32, i1) #0

As far as i can tell, there should be 2 declarations, something like:

declare void @llvm.memcpy.p1i8.p1i8.i32(i8*, i8 addrspace(1)* nocapture readonly, i32, i32, i1)
declare void @llvm.memcpy.p1i8.p1i8.i32(i8 addrspace(1)* nocapture, i8* readonly, i32, i32, i1)
yxsamliu commented 7 years ago

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.

AlexeySotkin commented 7 years ago

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? Moreover I don't see any benefits of using -O3, if we end up with the same SPIR-V code as we had with -O0.

yxsamliu commented 7 years ago

That can be a workaround if users are using Clang. I think it is better to be able to lower memmov.

doe300 commented 7 years ago

I need the other optimizations run with -O3, so disabling optimizations is not an option. I rewrote the pull-request using a lowering pass as suggested by @yxsamliu .

yxsamliu commented 7 years ago

There are too many commits in this pull request. Can you squash them into one commit? Otherwise LGTM.