-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix performance regression for device compileAndRun #63
Conversation
- Extract runtime constant types in the pass - Avoid bitcode parsing unless compiling - Link binaries for CUDA RDC in object codegen - Add kernel repeat test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This improves not only performance but also readability and simplifies the code IMO.
namespace proteus { | ||
|
||
enum RuntimeConstantTypes : int32_t { | ||
BOOL = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition, I should've done this in the first place
@@ -14,6 +14,19 @@ | |||
#include <cstring> | |||
#include <stdint.h> | |||
|
|||
namespace proteus { | |||
|
|||
enum RuntimeConstantTypes : int32_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why inherit from, int32_t, just to enforce size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to match the expected data type from the pass
auto getNumRCs() const { return NumRCs; } | ||
JITKernelInfo(char const *Name, int32_t *RCIndices, int32_t *RCTypes, | ||
int32_t NumRCs) | ||
: Name(Name), RCIndices{ArrayRef{RCIndices, static_cast<size_t>(NumRCs)}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice use of the constructors here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, solves my performance issue as well.
Closes #59