TTG/building system#424
Conversation
therault
force-pushed
the
ttg/building-system
branch
2 times, most recently
from
fb55ce7 to
562fbeb
Compare
| parsec_execution_stream_t *es, | ||
| const parsec_task_t* task); | ||
| typedef int (parsec_update_dependency_fn_t)(parsec_taskpool_t *tp, | ||
| const parsec_task_t* restrict task, |
There was a problem hiding this comment.
Just curious: why remove the restrict here?
There was a problem hiding this comment.
Because this .h file is pulled in by parsec/ttg.h, and restrict is a keyword that is refused by g++ and/or other C++ compilers, even if the declaration is within an extern "C" { ... } block. At least, this is what I remember of this issue.
There was a problem hiding this comment.
Ahh right, we need the header for parsec_task_s (and probably some other things). It's a bit ironic that an external project includes parsec_internal.h though ^^
There was a problem hiding this comment.
Some things will probably need cleaning, but I wouldn't say TTG is 'external' from PaRSEC... There is a shade area here :)
There was a problem hiding this comment.
I don't think the right approach is to remove this optimization keyword. Instead I would propose we protect restrict in parsec_config or parsec_config_bottom for C++ case, and leave it's usages in the C library as intended originally.
/* If we're in C++, redefine restrict to nothing */
#if defined(c_plusplus) || defined(__cplusplus)
# undef restrict
# define restrict
#endifThere was a problem hiding this comment.
The macro trick is now implemented.
There was a problem hiding this comment.
This undef of the macro will have side effects on users that include parsec.h
There was a problem hiding this comment.
Well, it will have a side effect only if the users are using a C++ compiler...
According to stackoverflow, some C++ compilers do support restrict or __restrict__... And others don't.
I guess we should have some CMake logic to decide if HAVE_RESTRICT or not, and have a corresponding cmakedefine.
CMake has a c_restrict low-level C compiler feature testable via target_compile_features, but there is no such thing in the C++ compiler feature set. I could try to compile some code with restrict with the C++ compiler to see if it's supported. But the macro we define would need to be conditional to the type of compiler anyway.
Comment below if you think I should implement this solution.
There was a problem hiding this comment.
Does anyone remember why restrict was added in the first place? We don't use it in other places where tp and tasks are passed, so maybe dropping it here is just ok?
There was a problem hiding this comment.
This is a function pointer and it cannot be inlined, so we could keep it on the C file (implementation), and remove it from this interface; as long as the compiler doesn't barf on the incompatible function type signatures I don't see what good the restrict does here.
therault
force-pushed
the
ttg/building-system
branch
from
97762f8 to
81b6f4e
Compare
|
Let's get this done. I agree with @abouteiller comment, restrict has little meaning in the function prototype, other than forcing all functions to support it. Instead, we can add it to the function implementation, allowing the compiler to do optimizations. Also, please squash all the commits into one or two (no more commits with revert). |
…e (restrict), protect some additional header files for C++
- Force PIC when compiling libparsec, even if it is static.
libttg.so links with libparsec.a... To allow this kind of behavior,
libparsec must be compiled in position independent code, even when
compiling it statically.
- Some Fixes to hwloc detection
Signed-off-by: Thomas Herault <herault@icl.utk.edu>
signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>
- Do not build the test target if parsec is used as a submodule
This is important for TTG, as the 'tests' goal gets overloaded,
when parsec is used as a submodule of TTG
Tests building is still not added to the target 'all' if parsec is
not the root project, but the parsec_addtest_cmd keeps doing the
add_test inconditionally
- Fix a typo introduced by cherry-picking very different versions
- Fix paths and other typos in compiler tests that are supposed to fail, so they fail for the right reason. Revert unproductive changes in CMakeLists.txt
- Missing variable initialization in user trigger termination detection, when in debug mode
- Use a macro in the PARSEC namespace to solve the restrict issue.
With this approach,
- if the user code is C, its compiler must be C99 compliant, and
restrict should be defined
- if the user code is C++, the user compiler might support restrict
or not, and we have no way to know that at the time we compile
PaRSEC (because the user might use another C++ compiler than the
one we detect). TTG, for one, requires to include parsec_internal.h,
so in this case we make sure restrict doesn't appear in the
prototype of these functions.
What we don't lose:
- the C99 compiler sees the restrict keywords and can generate
optimized code for the implementation of the corresponding
functions.
- We don't have side effects on the user code, even in the C++
case (if the user defines a variable named 'restrict' for
example, or if the user's compiler supports the keyword
restrict and they used it in other places).
What we lose:
- the C++ compiler doesn't see the restrict keyword, even if it
supports it, so if the user passes, by mistake, overlaping
pointers, their compiler will not issue a warning/error.
therault
force-pushed
the
ttg/building-system
branch
from
5881a03 to
08964d3
Compare
|
I rebased and squashed all commits. I fixed the comments.
…On Thu, Nov 3, 2022 at 8:48 PM bosilca ***@***.***> wrote:
Let's get this done. I agree with @abouteiller
<https://github.com/abouteiller> comment, restrict has little meaning in
the function prototype, other than forcing all functions to support it.
Instead, we can add it to the function implementation, allowing the
compiler to do optimizations. Also, please squash all the commits into one
or two (no more commits with revert).
—
Reply to this email directly, view it on GitHub
<#424 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFEZNXNLBHDRCLKVV2IMKTWGRMOHANCNFSM57B4XGIA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
4 participants
These commits are the ones missing for TTG support, which uses PaRSEC as a submodule.
There is one commit that still needs to be considered (not part of this PR):
352014fdbf10a141fe9f4eab7e70cac6949e638d [Restore the C11 support broken in 694d1e9 and few other things]. I think it was a backport from the master, but too many things have changed to make sure.
-- We need to check if this is still needed (or what parts of it are needed).