max with conflicting types

[chsigg]

Compiling gives an error 'deduced conflicting types for parameter '_Tp' ('unsigned int' vs. 'unsigned short')' at

nccl/src/enqueue.cu

Line 368 in 1450d42

info->comm->myParams->blockDim.x = max(info->comm->myParams->blockDim.x, coll.args.nThreads);

[sjeaugey]

Would a cast solve the issue ? e.g. (uint32_t)coll.args.nThreads ?

[chsigg]

Yes, a cast would work. Maybe specifying the template type is a little more common for this type of situation: max<unsigned>(...)

…

On Mon, Feb 11, 2019 at 6:33 PM Sylvain Jeaugey ***@***.***> wrote: Would a cast solve the issue ? e.g. (uint32_t)coll.args.nThreads ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHLOjgeUhEXkwWSM_t4aitpXGykPS8qYks5vMamDgaJpZM4a0ZaZ> .

[sjeaugey]

Actually, these kind of issues tend to happen frequently, so I was considering replacing all std::min and std::max by macros :

#define MIN(a, b) ((a)<(b) ? (a) : (b))
#define MAX(a, b) ((a)>(b) ? (a) : (b))

Do you see an advantage of using std:: functions instead of those trivial macros ?

[chsigg]

I'm fine with either approach, I don't see any real drawback of macros here, as long as they are not part of the public header. If anything, you might have to be careful to not include windows.h without defining NO_MIN_MAX. I have to admit that I'm not used to NCCL's code style, so you should go with your preference, not mine. Thanks for the quick response!

…

On Mon, Feb 11, 2019, 18:58 Sylvain Jeaugey ***@***.*** wrote: Actually, these kind of issued tend to happen frequently, so I was considering replacing all std::min and std::max by macros : #define MIN(a, b) ((a)<(b) ? (a) : (b)) #define MAX(a, b) ((a)>(b) ? (a) : (b)) Do you see an advantage of using std:: functions instead of those trivial macros ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHLOjkf5sVsd8hxucgi9jIKR4jq6wqHIks5vMa81gaJpZM4a0ZaZ> .

[sjeaugey]

Actually those macros would evaluate the selected expression twice. I guess that's the main problem with that approach since it could cause weird bugs.

For example, a = MAX(b, ++a); might make a equal to a+1 or a+2 if a+1 is larger than b.

[AddyLaddy]

Fixes for this issue have been made to NCCL 2.4.6. Please re-open if the issue is not resolved.