Brian, CC'ing mpich2-core for their comments. I agree that using MPIDI_Exit is just tricking the compiler, not really solving the problem. But, I don't know which way is a better approach --- to go for the MPIDI_Exit form where we trick the compiler, or use pragmas for *each* "problematic" compiler and thus making the code unnecessarily complicated. Regarding the Segment_manipulate code, this is already the 4th revision that Rob has done, so it's close to the cleanest of what we could get while retaining the performance :-). It's an amazingly complicated function, but has been optimized a ton. I tried breaking it down to smaller functions earlier, but you won't believe how easily we lose performance when I tried to do that. Anyway, I can go either way with the pragma stuff depending on what the rest of the MPICH2 group feels. Thanks. -- Pavan PS: Will fix the simple_pmi.c warning. On 01/14/2008 09:48 AM, Brian Curtis wrote:
Pavan,
Those changes do obfusticate the use of noreturn functions, thus tricking the compiler into believing certain lines of code are reachable. However, this approach will lead to unintended consequences as it also will trick developers. By adding the MPIDI_Exit function without the noreturn designation, you have created a facade for the noreturn exit functions. I understand the intent of the change, but I'd much prefer the use of #pragma's in this case. Also, the changes added the following warning:
"simple_pmi.c", line 353: warning: implicit function declaration: MPIDI_Exit
As for segment.c, the "void PREPEND_PREFIX(Segment_manipulate)" function contains a giant "for(;;)" loop, with no means of exiting the loop with the exception of returns. We are using the pragma's to prevent the compiler from warning us about the lines of code that follow this for loop. Part of the reason this type of code structure pops up is the size of the function in question--can this be refactored?
Will you consider reverting these changes and using the pragma's I provided, instead?
Regards,
Brian
Pavan Balaji wrote:
Brian,
Can you try the current version of the trunk and see if this problem still exists?
./src/mpid/common/datatype/dataloop/segment.c
Btw, I couldn't find the problem in this file (maybe I'm too tired). Can you point me to where exactly the error is showing up?
Thanks.
-- Pavan
-- Pavan Balaji http://www.mcs.anl.gov/~balaji
I think that the right thing to do is MPIDI_Exit(); /* this function does not return */ That is easiest for the developer (simpler than a large number of vendor-specific pragmas) and keeps the compiler from generating an undesired warning. Note that under Windows, you want to call ExitProcess, not exit, and so it is actually better to call a routine with an internal but obvious name rather than use the #if windows #define exit(_e) ExitProcess(_e) #endif which we have to use right now. Bill On Jan 14, 2008, at 11:15 AM, Pavan Balaji wrote:
Brian,
CC'ing mpich2-core for their comments.
I agree that using MPIDI_Exit is just tricking the compiler, not really solving the problem. But, I don't know which way is a better approach --- to go for the MPIDI_Exit form where we trick the compiler, or use pragmas for *each* "problematic" compiler and thus making the code unnecessarily complicated.
Regarding the Segment_manipulate code, this is already the 4th revision that Rob has done, so it's close to the cleanest of what we could get while retaining the performance :-). It's an amazingly complicated function, but has been optimized a ton. I tried breaking it down to smaller functions earlier, but you won't believe how easily we lose performance when I tried to do that.
Anyway, I can go either way with the pragma stuff depending on what the rest of the MPICH2 group feels.
Thanks.
-- Pavan
PS: Will fix the simple_pmi.c warning.
On 01/14/2008 09:48 AM, Brian Curtis wrote:
Pavan, Those changes do obfusticate the use of noreturn functions, thus tricking the compiler into believing certain lines of code are reachable. However, this approach will lead to unintended consequences as it also will trick developers. By adding the MPIDI_Exit function without the noreturn designation, you have created a facade for the noreturn exit functions. I understand the intent of the change, but I'd much prefer the use of #pragma's in this case. Also, the changes added the following warning: "simple_pmi.c", line 353: warning: implicit function declaration: MPIDI_Exit As for segment.c, the "void PREPEND_PREFIX(Segment_manipulate)" function contains a giant "for(;;)" loop, with no means of exiting the loop with the exception of returns. We are using the pragma's to prevent the compiler from warning us about the lines of code that follow this for loop. Part of the reason this type of code structure pops up is the size of the function in question--can this be refactored? Will you consider reverting these changes and using the pragma's I provided, instead? Regards, Brian Pavan Balaji wrote:
Brian,
Can you try the current version of the trunk and see if this problem still exists?
./src/mpid/common/datatype/dataloop/segment.c
Btw, I couldn't find the problem in this file (maybe I'm too tired). Can you point me to where exactly the error is showing up?
Thanks.
-- Pavan
-- Pavan Balaji http://www.mcs.anl.gov/~balaji
William Gropp Paul and Cynthia Saylor Professor of Computer Science University of Illinois Urbana-Champaign
Assuming the interface for the abort cannot be changed and that a compiler/static analysis tool (I've not tried this) does not (now or eventually) pick up on the trick, Bill's suggestion is reasonable. It's important to point out that this trick works now, but could lead to using pragma's in the future. For now, the Sun Studio compiler assumes that functions not designated as noreturn, actually return. Brian William Gropp wrote:
I think that the right thing to do is
MPIDI_Exit(); /* this function does not return */
That is easiest for the developer (simpler than a large number of vendor-specific pragmas) and keeps the compiler from generating an undesired warning. Note that under Windows, you want to call ExitProcess, not exit, and so it is actually better to call a routine with an internal but obvious name rather than use the
#if windows #define exit(_e) ExitProcess(_e) #endif
which we have to use right now.
Bill
On Jan 14, 2008, at 11:15 AM, Pavan Balaji wrote:
Brian,
CC'ing mpich2-core for their comments.
I agree that using MPIDI_Exit is just tricking the compiler, not really solving the problem. But, I don't know which way is a better approach --- to go for the MPIDI_Exit form where we trick the compiler, or use pragmas for *each* "problematic" compiler and thus making the code unnecessarily complicated.
Regarding the Segment_manipulate code, this is already the 4th revision that Rob has done, so it's close to the cleanest of what we could get while retaining the performance :-). It's an amazingly complicated function, but has been optimized a ton. I tried breaking it down to smaller functions earlier, but you won't believe how easily we lose performance when I tried to do that.
Anyway, I can go either way with the pragma stuff depending on what the rest of the MPICH2 group feels.
Thanks.
-- Pavan
PS: Will fix the simple_pmi.c warning.
On 01/14/2008 09:48 AM, Brian Curtis wrote:
Pavan, Those changes do obfusticate the use of noreturn functions, thus tricking the compiler into believing certain lines of code are reachable. However, this approach will lead to unintended consequences as it also will trick developers. By adding the MPIDI_Exit function without the noreturn designation, you have created a facade for the noreturn exit functions. I understand the intent of the change, but I'd much prefer the use of #pragma's in this case. Also, the changes added the following warning: "simple_pmi.c", line 353: warning: implicit function declaration: MPIDI_Exit As for segment.c, the "void PREPEND_PREFIX(Segment_manipulate)" function contains a giant "for(;;)" loop, with no means of exiting the loop with the exception of returns. We are using the pragma's to prevent the compiler from warning us about the lines of code that follow this for loop. Part of the reason this type of code structure pops up is the size of the function in question--can this be refactored? Will you consider reverting these changes and using the pragma's I provided, instead? Regards, Brian Pavan Balaji wrote:
Brian,
Can you try the current version of the trunk and see if this problem still exists?
./src/mpid/common/datatype/dataloop/segment.c
Btw, I couldn't find the problem in this file (maybe I'm too tired). Can you point me to where exactly the error is showing up?
Thanks.
-- Pavan
-- Pavan Balaji http://www.mcs.anl.gov/~balaji
William Gropp Paul and Cynthia Saylor Professor of Computer Science University of Illinois Urbana-Champaign
participants (3)
-
Brian Curtis -
Pavan Balaji -
William Gropp