1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

Rename autoconfigured CCTK_BUILTIN functions to __builtin

Issue #1379 closed
Erik Schnetter created an issue

With autoconf, it is common to define a work-around if a tested feature does not exist. For example, when the "inline" keyword does not exist, then there is a #define that allows code to still use "inline", instead of requiring all code to use CCTK_INLINE. This keeps code more readable.

Cactus tests for various __builtin_ features (__builtin_expect, __builtin_unreachable). If these are not present, it defines CCTK_BUILTIN work-around that one can use instead. I suggest to provide instead the respective __builtin_ functions.

We would keep the current CCTK_BUILTIN_* variants for backward compatibility (at least for some time), but would not define them for new builtins that we autoconfigure.

$ svn diff cctk_Config.h.in
Index: cctk_Config.h.in
===================================================================
--- cctk_Config.h.in    (revision 5021)
+++ cctk_Config.h.in    (working copy)
@@ -376,6 +376,7 @@
 #  define CCTK_BUILTIN_EXPECT(x,y) __builtin_expect(x,y)
 #else
 #  define CCTK_BUILTIN_EXPECT(x,y) (x)
+#  define __builtin_expect(x,y) CCTK_BUILTIN_EXPECT(x,y)
 #endif

 /* Whether __builtin_unreachable exists. */
@@ -384,8 +385,15 @@
 #  define CCTK_BUILTIN_UNREACHABLE() __builtin_unreachable()
 #else
 #  define CCTK_BUILTIN_UNREACHABLE() CCTK_Abort(0, 1)
+#  define __builtin_unreachable() CCTK_BUILTIN_UNREACHABLE()
 #endif

 /* OpenMP collapse clause */
 #if (defined CCTK_DISABLE_OMP_COLLAPSE ||                               \
      (defined __IBMC__ && defined _ARCH_450D) ||                        \
@@ -585,6 +593,7 @@
 #  define CCTK_BUILTIN_EXPECT(x,y) __builtin_expect(x,y)
 #else
 #  define CCTK_BUILTIN_EXPECT(x,y) (x)
+#  define __builtin_expect(x,y) CCTK_BUILTIN_EXPECT(x,y)
 #endif

 /* Whether __builtin_unreachable exists. */
@@ -593,8 +602,15 @@
 #  define CCTK_BUILTIN_UNREACHABLE() __builtin_unreachable()
 #else
 #  define CCTK_BUILTIN_UNREACHABLE() CCTK_Abort(0, 1)
+#  define __builtin_unreachable() CCTK_BUILTIN_UNREACHABLE()
 #endif

 /* Whether static_assert exists. */
 #undef HAVE_CCTK_CXX_STATIC_ASSERT
 #ifdef HAVE_CCTK_CXX_STATIC_ASSERT

Keyword:

Comments (7)

  3. Ian Hinder
    • removed comment

    The recent update to hwloc fails to build on Datura, and Erik says that this patch is required to fix this.

    My thoughts on this are the following, but I don't have a well-informed opinion at the moment; please correct me if I make any mistakes!

    Is the use of __builtin_expect (for example) the recommended way to annotate the code? Should we be using compiler-specific features like this? Does this make the code gcc-specific? I'm tempted to disagree with Cactus providing functions which are usually provided by GCC on principle. Was there a specific incident that prompted this, e.g. hwloc expecting that this was available? Does this mean that hwloc is compatible only with GCC and not the Intel compiler, where this is not provided? I wonder if it would be better not to interfere with compiler-provided names, and instead stick with the CCTK_ prefix for features that we are providing in the "Cactus platform". So code written "for" Cactus will use the CCTK_ version (and will be guaranteed to work wherever Cactus is supported), rather than something which is associated with GCC, and code which is imported in bulk from somewhere else is buggy if it only works with GCC, and needs a specific workaround (e.g. we can define the GCC __builtin_expect as the CCTK_ version for that library, which limits the scope of the change to the code which is not portable, rather than modifying what might be expected behaviour globally).

    This is in principle the same as the M_PI issue, but there we can make the case that M_PI is pervasively expected in numerical code, and argue from practicality that defining it ourselves is OK, whereas the GCC __builtin_* functions are not so widely used, and there it might cause confusion and/or problems to define them in Cactus.

  4. Ian Hinder
    • removed comment

    Erik and I just discussed this, but didn't come to a conclusion. __builtin_expect is a gcc extension which is supported by some other compilers. As such, it is probably safe in practice for us to do as Erik suggests, and define it if it doesn't exist. However, I think we would be technically breaching the standards by doing this, as we are not a compiler implementation, so should not be defining things with double-underscore prefixes (according to the ANSI C standard). Erik also points out that we breach the standards in many other ways as well. Apart from this specific case, I think we should decide on a policy for this sort of thing. I am leaning towards the opinion that Cactus should only define things in its own namespace, rather than providing features from some compilers on others that users might expect. This touches on features such as "inline", "restrict" and "M_PI"; all of these are things that Cactus tries to define according to common usage or another standard, when the currently-compiled standard doesn't provide them. Issues of compatibility and portability seem to be complicated enough without Cactus defining symbols like this. The logical conclusion of this would be that Cactus defines CCTK_EXPECT, CCTK_INLINE, CCTK_RESTRICT and CCTK_PI as part of the "Cactus platform", and we guarantee that this will work everywhere Cactus is supported. It is then clear to the reader of the code that they are using a feature provided on some level by Cactus, rather than something which looks like a compiler-specific feature but is actually provided by Cactus. The resulting code looks more ugly (in the case of inline, restrict and M_PI, though arguably not in the case of __builtin_expect, but we have to deal with CCTK_ARGUMENTS, CCTK_REAL etc anyway, and at least it is clear when Cactus is involved.

    Since the ET currently doesn't build on Datura, Erik and I agreed that he would just use the CCTK_ prefix for now, and defer the decision on policy until more people have weighed in.

  5. Roland Haas
    • removed comment

    I agree with Ian in not liking Cactus re-defining reserved words of the language ("inline", "restrict") and possibly double (or single which are reserved for the C standard library for that matter) underscored symbols. I am not sure I buy into the "looks ugly" argument. If the issue is only syntax highlighting, then it is usually simple to extent one's favorite editor's syntax files to cope with CCTK_XXX items (and eg CCTK_REAL, CCTK_INT etc as well). My thoughts about M_PI which is a macro to begin with are less strong, there I don't mind if Cactus provides a definition of the environment does not (as long as it is the correct value of pi).

    So I'd be happy having all of these things be CCTK_STATIC_INLINE CCTK_RESTRICT etc again. Of course this means that we are re-inventing the wheel as far as names goes.

  6. Erik Schnetter reporter
    • changed status to resolved
    • removed comment

    We are not going to require people to write CCTK_INLINE instead of inline.

    Apart from this, the discussion came to a conclusion.

  8. Log in to comment
Assignee
–
Type
enhancement
Priority
minor
Status
closed
Component
Cactus
Milestone
–
Version
development version
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!