You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/06/08 16:33:15 UTC
[GitHub] [tvm] kparzysz-quic opened a new pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
kparzysz-quic opened a new pull request #8213:
URL: https://github.com/apache/tvm/pull/8213
- Generate valid LLVM IR.
- Set proper alignment on the constant variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856930673
The generated LLVM IR was not valid: for example it had bitcasts between pointer and array types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r649274314
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
ok, just to clarify here--you're saying it's unnecessary because the ultimate effect of dereferencing the pointer is the same?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r647788391
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
No, there is no need for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ANSHUMAN87 commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856928445
> * Generate valid LLVM IR.
> * Set proper alignment on the constant variables.
@kparzysz-quic : could you please describe what was the issue? And possibly scenario to reproduce the issue. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-858897098
thanks @kparzysz-quic , the PR is now merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-857150051
The test `tests/python/unittest/test_link_params.py` fails without this patch. How come it wasn't failing in the CI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-858034729
I've been using the entire clang toolchain built with assertions (the Release+Assertions build) in my daily work and I don't find it too slow. Frankly, I'm not sure if I could tell whether I'm running Release or Release+Assertions without some known references or comparing them side by side. I wouldn't be concerned at all about using such a build for CI. The slowness issue would definitely come up with a Debug build, which AFAIR is unoptimized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r649388810
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
To add more to this: in order to load or store something, we need the address of it, and that address is usually obtained by `getelementptr`. In this case we already have the address of the location we want to store to, so the type casting and `getelementptr` use is completely redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ANSHUMAN87 commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856950542
> The generated LLVM IR was not valid: for example it had bitcasts between pointer and array types.
Thanks for your response. Will it be possible for you to share one small snippets which will show LLVM IR before your change where the issue lies, and after your fix the resulting LLVM IR where the issue is resolved. TIA!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-858681736
What else needs to be done for this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r647767341
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
`int[n]` in LLVM is `[n x i32]`, and it's an aggregate type, i.e. it's the entire array. If you want to load or store an element of it, you need the address of the beginning of it, i.e. `i32*`. Unlike in C, arrays in LLVM IR are not treated as pointers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856963214
This is the assertion that I'm getting without this patch:
```
python3: /w/src/llvm-project/llvm/lib/IR/Type.cpp:689: static llvm::PointerType *llvm::PointerType::get(llvm::Type *, unsigned int): Assertion `isValidElementType(EltTy) && "Invalid type for pointer element!"' failed.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ANSHUMAN87 commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-857002997
Thanks a lot @kparzysz-quic for clarification 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r647737507
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
i'm really the furthest from an expert at LLVM bitcode, but it seems like this stops treating ret_tcode as `int[]` and merely as the int* it is. is that bad?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856972609
The above assertion is because the program tried to get a pointer to `void` in LLVM. There is no such thing, hence the assertion. After you fix that, you hit another problem: the bitcast that was on line 221 was casting `i8* %0` to `[1 x i8*]`, which is not a legal cast, and so you get another assertion:
```
python3: /w/src/llvm-project/llvm/lib/IR/Instructions.cpp:2984: static llvm::CastInst *llvm::CastInst::Create(Instruction::CastOps, llvm::Value *, llvm::Type *, const llvm::Twine &, llvm::Instruction *): Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-858705795
ok, just wanted to understand those points. it seems like we should modify our LLVM install to build with assertions on in at least one version in one container. that doesn't have to block this PR, though. could you just clarify your answer to the one question about GEP and we can merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-857166287
I think I know what's going on: if you use LLVM that was compiled without assertions, it may not crash at the time of generating the LLVM IR. I guess that the generated LLVM IR ends up working correctly, at least in this case.
I use LLVM with built-in assertions, so for me it crashes. I don't know if there is a reliable way to make the current code fail without using assertions in LLVM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r647785347
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
right, so given ret_tcode is `int[1]`, should we be representing that in its llvm type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r649294974
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
Something like that... There is really no benefit of creating a `[1 x i32]` type here. Using aggregate types allows LLVM IR to express the address computations in a target-independent way (via `getelementptr`), but arrays are always contiguous, so loading the 0th element of `[1 x i32]` is the same as loading from `i32*` that is the address of that element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-857824746
@kparzysz-quic yeah i think you're right--we get LLVM from APT in the docker container, which i doubt has `-DLLVM_ENABLE_ASSERTIONS` as the website mentions it's slow. For that reason, I'm hesitant to suggest changing our LLVM install process to build one with assertions as it might significantly impact the CI speed. we could potentially add one version which contains assertions and run codegen unit tests against that. what are your thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8213:
URL: https://github.com/apache/tvm/pull/8213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r649435369
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
thanks for the great explanation @kparzysz-quic !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on a change in pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#discussion_r649302309
##########
File path: src/target/llvm/codegen_llvm.cc
##########
@@ -215,41 +205,29 @@ void CodeGenLLVM::LinkParameters(const Map<String, LinkedParam> params) {
llvm::BasicBlock* entry = llvm::BasicBlock::Create(*ctx_, "entry", function);
builder_->SetInsertPoint(entry);
- std::vector<llvm::Value*> zero_index_list{llvm::ConstantInt::get(t_int32_, 0)};
- std::vector<llvm::Value*> zero_array_index_list{llvm::ConstantInt::get(t_int32_, 0),
- llvm::ConstantInt::get(t_int32_, 0)};
- auto args_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[0],
+
+ auto getArg = [function](int i) -> llvm::Argument* {
+#if TVM_LLVM_VERSION >= 100
+ return function->getArg(i);
+#elif TVM_LLVM_VERSION >= 50
+ return &function->arg_begin()[i];
#else
- &(*(function->arg_begin())),
+ return &*std::next(function->arg_begin(), i);
#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1));
- llvm::Value* sid = builder_->CreateBitCast(
- builder_->CreateLoad(t_void_->getPointerTo(GetGlobalAddressSpace()),
- builder_->CreateInBoundsGEP(args_array, zero_index_list)),
- t_int64_);
+ };
+
+ llvm::Type* t_int64_p = t_int64_->getPointerTo(GetGlobalAddressSpace());
+ llvm::Value* sid = builder_->CreateLoad(t_int64_, builder_->CreateBitCast(getArg(0), t_int64_p));
+
+ auto ret_tcode = builder_->CreateBitCast(getArg(4), t_int_p);
+ auto ret_value =
+ builder_->CreateBitCast(getArg(3), t_void_p_->getPointerTo(GetGlobalAddressSpace()));
llvm::BasicBlock* default_block = llvm::BasicBlock::Create(*ctx_, "default_block", function);
- auto ret_types_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[4],
-#else
- &(*(std::next(function->arg_begin(), 4))),
-#endif
- llvm::ArrayType::get(t_int_, 1)->getPointerTo());
- auto retval_array = builder_->CreateBitCast(
-#if TVM_LLVM_VERSION >= 50
- &function->arg_begin()[3],
-#else
- &(*std::next(function->arg_begin(), 3)),
-#endif
- llvm::ArrayType::get(t_void_->getPointerTo(GetGlobalAddressSpace()), 1)->getPointerTo());
llvm::SwitchInst* switch_inst = builder_->CreateSwitch(sid, default_block, params.size() + 1);
builder_->SetInsertPoint(default_block);
- builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr),
- builder_->CreateInBoundsGEP(ret_types_array, zero_array_index_list));
+ builder_->CreateStore(llvm::ConstantInt::get(t_int_, kTVMNullptr), ret_tcode);
Review comment:
In fact, in this code
```
define i32 @fred(i32* %a0) {
%v0 = bitcast i32* %a0 to [1 x i32]*
%v1 = getelementptr [1 x i32], [1 x i32]* %v0, i32 0, i32 0
%v2 = load i32, i32* %v1, align 4
ret i32 %v2
}
```
the use of the array type gets optimized out (in `instcombine`):
```
$ opt -S -instcombine < 1xi32.ll
; ModuleID = '<stdin>'
source_filename = "<stdin>"
define i32 @fred(i32* %a0) {
%v2 = load i32, i32* %a0, align 4
ret i32 %v2
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kparzysz-quic commented on pull request #8213: [LLVM] Fix CodeGenLLVM::LinkParameters
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #8213:
URL: https://github.com/apache/tvm/pull/8213#issuecomment-856956704
If you use LLVM with assertions enabled, it will simply not allow you to create such a bitcast: attempt to do so will trigger an assertion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org