Fix MFU printing #585
Fix MFU printing #585
Conversation
train_gpt2.cu
Outdated
| @@ -1671,6 +1672,7 @@ int main(int argc, char *argv[]) { | |||
| } | |||
|
|
|||
| // train | |||
| char* mfu_str = (char*)malloc(16); | |||
There was a problem hiding this comment.
I'm a bit nervous about introducing this, hardcoding 16 here, having to free it, etc.
There was a problem hiding this comment.
I agree there is a slight overhead but this is done once and left. Plus even if we had a leak it's 16 bytes :D
There was a problem hiding this comment.
Lmk if you have a better proposal! I still think in net it's better than having -100% for non-supported GPUs.
There was a problem hiding this comment.
can't we just put these 16 bytes on the stack?
There was a problem hiding this comment.
definitely - that's cleaner!
|
char* makes me nervous. i'm not actually sure if the -100% is terrible... or we can make it 0% if it looks less scary. |
|
One method would be to define a constant MAX_STRING_SIZE - then you would set mfu_str = malloc (MAX_STRING_SIZE) and use it as the second param in the snprintf statement - snprintf(mfu_str, MAX_STRING_SIZE, "n/a"); You would need to free it too when done. If you don't malloc it - you would still need the constant for the definition mfu_str[MAX_STRING_SIZE] in order to use it elsewhere in the file when using snprintf(mfu_str, MAX_STRING_SIZE, "n/a");. sizeof() only works correctly in the same function where the mfu_str char array is declared. I would use different names too. Using the same name makes it a bit confusing to me. |
We have a bug when the device we're running on is not supported: we print "-100%". Fixed it so we print "n/a" in that case.
Also added support for H100 PCIe which is the device I have.