Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LLVM] Fix generation of LLVM intrinsics #5282

Merged
merged 3 commits into from Apr 11, 2020
Merged

[LLVM] Fix generation of LLVM intrinsics #5282

merged 3 commits into from Apr 11, 2020

Conversation

kparzysz-quic
Copy link
Contributor

The type list in the call to llvm::Intrinsic::getDeclaration is not the intrinsic's signature, it's the list of overloaded types. Without this fix, the updated unit test would cause the following error:

TVMError: LLVM module verification failed with the following errors:
Intrinsic name not mangled correctly for type arguments! Should be: llvm.ctlz.i32
i32 (i32, i1)* @llvm.ctlz.i32.i1

@tqchen
Copy link
Member

tqchen commented Apr 8, 2020

cc @kparzysz-quic please look into the CI problem

@kparzysz-quic
Copy link
Contributor Author

Yes, I'm working on it.

@tqchen tqchen added the status: need update need update based on feedbacks label Apr 8, 2020
@kparzysz-quic
Copy link
Contributor Author

I guess I didn't get everything...

@@ -698,21 +698,33 @@ llvm::Value* CodeGenLLVM::CreateIntrinsic(const CallNode* op) {
sig_type.push_back(arg_value.back()->getType());
}
}
// LLVM's prefetch intrinsic returns "void", while TVM's prefetch
// returns int32. This causes problems because prefetch is one of
// those intrinsics that is generated automatically via the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, let us leave a TODO here, hopefully after we introduce the type system to type the intrinsics, we will be able to use the same type as LLVM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@kparzysz-quic
Copy link
Contributor Author

The entire tests/python/unittest/test_target_codegen_llvm.py passes for me, I don't know what's going on. I'll look into it tomorrow.

@tqchen
Copy link
Member

tqchen commented Apr 9, 2020

The CI error was due to a flaky problem which is fixed now by #5293

@kparzysz-quic
Copy link
Contributor Author

@tqchen
Copy link
Member

tqchen commented Apr 9, 2020

OK #5297 temporary disabled the crt test

Krzysztof Parzyszek added 3 commits April 10, 2020 13:45
The type list in the call to llvm::Intrinsic::getDeclaration is not
the intrinsic's signature, it's the list of overloaded types. Without
this fix, the updated unit test would cause the following error:

TVMError: LLVM module verification failed with the following errors:
Intrinsic name not mangled correctly for type arguments! Should be:
llvm.ctlz.i32
i32 (i32, i1)* @llvm.ctlz.i32.i1

Special handling for llvm.prefetch, sig matching for overloaded ints only

The prefetch intrinsic returns void in LLVM, while it returns i32 in TVM.
This case needs to be handled specially, because rule-based intrinsic
translation would cause invalid LLVM type to be created.

Do the signature matching only for overloaded intrinsics. It's not needed
for non-overloaded ones, so this can save a bit of compile-time.
@tqchen tqchen removed the status: need update need update based on feedbacks label Apr 11, 2020
@tqchen tqchen merged commit 403929f into apache:master Apr 11, 2020
@kparzysz-quic kparzysz-quic deleted the overload branch April 11, 2020 15:22
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
* [LLVM] Fix generation of LLVM intrinsics

The type list in the call to llvm::Intrinsic::getDeclaration is not
the intrinsic's signature, it's the list of overloaded types. Without
this fix, the updated unit test would cause the following error:

TVMError: LLVM module verification failed with the following errors:
Intrinsic name not mangled correctly for type arguments! Should be:
llvm.ctlz.i32
i32 (i32, i1)* @llvm.ctlz.i32.i1

Special handling for llvm.prefetch, sig matching for overloaded ints only

The prefetch intrinsic returns void in LLVM, while it returns i32 in TVM.
This case needs to be handled specially, because rule-based intrinsic
translation would cause invalid LLVM type to be created.

Do the signature matching only for overloaded intrinsics. It's not needed
for non-overloaded ones, so this can save a bit of compile-time.

* Include intrinsic name in the error message

* Fix number of arguments for llvm.fmuladd and llvm.pow
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
* [LLVM] Fix generation of LLVM intrinsics

The type list in the call to llvm::Intrinsic::getDeclaration is not
the intrinsic's signature, it's the list of overloaded types. Without
this fix, the updated unit test would cause the following error:

TVMError: LLVM module verification failed with the following errors:
Intrinsic name not mangled correctly for type arguments! Should be:
llvm.ctlz.i32
i32 (i32, i1)* @llvm.ctlz.i32.i1

Special handling for llvm.prefetch, sig matching for overloaded ints only

The prefetch intrinsic returns void in LLVM, while it returns i32 in TVM.
This case needs to be handled specially, because rule-based intrinsic
translation would cause invalid LLVM type to be created.

Do the signature matching only for overloaded intrinsics. It's not needed
for non-overloaded ones, so this can save a bit of compile-time.

* Include intrinsic name in the error message

* Fix number of arguments for llvm.fmuladd and llvm.pow
dpankratz pushed a commit to dpankratz/incubator-tvm that referenced this pull request Apr 24, 2020
* [LLVM] Fix generation of LLVM intrinsics

The type list in the call to llvm::Intrinsic::getDeclaration is not
the intrinsic's signature, it's the list of overloaded types. Without
this fix, the updated unit test would cause the following error:

TVMError: LLVM module verification failed with the following errors:
Intrinsic name not mangled correctly for type arguments! Should be:
llvm.ctlz.i32
i32 (i32, i1)* @llvm.ctlz.i32.i1

Special handling for llvm.prefetch, sig matching for overloaded ints only

The prefetch intrinsic returns void in LLVM, while it returns i32 in TVM.
This case needs to be handled specially, because rule-based intrinsic
translation would cause invalid LLVM type to be created.

Do the signature matching only for overloaded intrinsics. It's not needed
for non-overloaded ones, so this can save a bit of compile-time.

* Include intrinsic name in the error message

* Fix number of arguments for llvm.fmuladd and llvm.pow
monklof pushed a commit to monklof/incubator-tvm that referenced this pull request Jan 22, 2021
…m_data:master to master

* commit 'cd0d52daa6942bdafa9363ff6cfa3d25fcd5b8d6': (824 commits)
  [Intrinsic] Add log1p, ldexp, atan2, hypot, nextafter, copysign (apache#5312)
  [Rust][CI] Restore Rust CI (apache#5137)
  Remove PrimExpr from String (apache#5311)
  [Requantize] Cleanup and Optimize Lowering (apache#5286)
  [IR][TRANSFORM] Enable CopyOnWrite for passes. (apache#5309)
  [PYTORCH]Abs, Arange, Softplus ops (apache#5295)
  [LLVM] Fix generation of LLVM intrinsics (apache#5282)
  [BYOC] Add example of Composite + Annotate for DNNL fused op (apache#5272)
  [Frontend][TensorFlow]Improve TensorFlow Static Shape Tensor Array (apache#5243)
  [RUNTIME] Introduce RValue reference(move) support to TypedPackedFunc (apache#5271)
  [RELAY][FRONTEND][CAFFE2] add Mul and ConvTranspose operator (apache#5302)
  [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes (apache#5277)
  [CI] Fix the hexagon string (apache#5304)
  [Arith] linear system and equation solver (apache#5171)
  [PYTORCH]Repeat, Reciprocal & Reshape Op support (apache#5280)
  [FRONTEND][TENSORFLOW] Fix gather_nd indices (apache#5279)
  Update device_annotation.cc (apache#5291)
  [REFACTOR][IR] Move to runtime::String (apache#5276)
  [NDArray] Set shape_ in NDArray::FromDLPack (apache#5301)
  [RUNTIME] Initial implementation of Hexagon runtime support (apache#5252)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants