diff mbox series

[RFC,11/12] Add explicit bool casts to .md condition users

Message ID 13c680ff-cf5b-f5fb-c101-d2c45fe3414f@e124511.cambridge.arm.com
State New
Headers show
Series aarch64: Extend aarch64_feature_flags to 128 bits | expand

Commit Message

Andrew Carlotti May 14, 2024, 2:59 p.m. UTC 
This patch is one way to fix some issues I discovered when disallowing
implicit casts to bool from aarch64_feature_flags (in a later patch).
That in turn was necessary to prohibit accidental conversion of an
aarch64_feature_flags value to an integer by first implicitly casting to
a bool (and thus setting the resulting integer value to 0 or 1).

Most of the uses of TARGET_ macros occur indirectly in middle end code,
via their use in instruction pattern conditions.  There are also a few
uses in aarch64 backend code, which are also changed in this patch.

The documentation on instruction patterns [1] doesn't explicitly say
that the condition must be a bool.  If we want to assume this, I think
we should update the documentation, and ideally enforce type consistency
within the compiler.

The code generated in genconditions.cc by write_one_condition already
includes an assumption that casting a condition's value to an int is
valid (i.e. that it does not invoke undefined behaviour, and does not
change the result obtained when later converting it to a boolean
result).  Fortunately, for aarch64 at least, this assumption only needs
to hold when the original constant is a compile time constant, whereas
all our problematic usage involves comparisons against the runtime
feature mask.

If the use of non-bool instruction pattern conditions should be
disallowed, then it would be straightforward to fix the type mismatches
in the aarch64 backend, by adding explicit bool casts to all of the
TARGET_* macros.  Indeed, I think that would be a better approach to
fixing this issue.  However, I felt it would be more useful to first
investigate and demonstrate the downstream impact of these type issues.

Note that this patch doesn't compile without the subsequent patch,
due to ambiguous calls to aarch64_def_or_undef(int, ...).  I expect to
replace this patch with one that avoids the issue, so it isn't worth
meddling with the next patch in the series just to make this RFC compile
by itself.

[1] https://gcc.gnu.org/onlinedocs/gccint/Patterns.html
diff mbox series

Patch

diff --git a/gcc/c-family/c-cppbuiltin.cc b/gcc/c-family/c-cppbuiltin.cc
index b6f25e4db3c06a1addc09a47335fe5184cb4a100..0cfcac6ba6b1e0ae7cdc0fb864eb28ec7de78605 100644
--- a/gcc/c-family/c-cppbuiltin.cc
+++ b/gcc/c-family/c-cppbuiltin.cc
@@ -1506,7 +1506,7 @@  c_cpp_builtins (cpp_reader *pfile)
 
 #ifdef HAVE_adddf3
 	  builtin_define_with_int_value ("__LIBGCC_HAVE_HWDBL__",
-					 HAVE_adddf3);
+					 (bool) HAVE_adddf3);
 #endif
 	}
 
diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
index fe1a20e4e546a68e5f7eddff3bbb0d3e831fbd9b..de4b383cda92c160bd706f9085999daac5d8313a 100644
--- a/gcc/config/aarch64/aarch64-c.cc
+++ b/gcc/config/aarch64/aarch64-c.cc
@@ -47,6 +47,12 @@  aarch64_def_or_undef (bool def_p, const char *macro, cpp_reader *pfile)
     cpp_undef (pfile, macro);
 }
 
+static void
+aarch64_def_or_undef (aarch64_feature_flags def_p, const char *macro, cpp_reader *pfile)
+{
+  aarch64_def_or_undef ((bool) def_p, macro, pfile);
+}
+
 /* Define the macros that we always expect to have on AArch64.  */
 
 static void
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 69c3b257982b4a0e282cbf7486802b147d166945..052cf297e7672abf015a085ab357836cb3b235e4 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -6561,10 +6561,10 @@  aarch64_function_value_regno_p (const unsigned int regno)
   /* Up to four fp/simd registers can return a function value, e.g. a
      homogeneous floating-point aggregate having four members.  */
   if (regno >= V0_REGNUM && regno < V0_REGNUM + HA_MAX_NUM_FLDS)
-    return TARGET_FLOAT;
+    return (bool) TARGET_FLOAT;
 
   if (regno >= P0_REGNUM && regno < P0_REGNUM + HA_MAX_NUM_FLDS)
-    return TARGET_SVE;
+    return (bool) TARGET_SVE;
 
   return false;
 }
diff --git a/gcc/genconditions.cc b/gcc/genconditions.cc
index 13963dc3ff46aa250c39ce80d0b92356390e41ff..3aee4428ff7ff5c97260f56a5f6b0fffa4e95fc2 100644
--- a/gcc/genconditions.cc
+++ b/gcc/genconditions.cc
@@ -140,9 +140,9 @@  write_one_condition (void **slot, void * ARG_UNUSED (dummy))
       putchar (*p);
     }
 
-  fputs ("\",\n    __builtin_constant_p ", stdout);
+  fputs ("\",\n    __builtin_constant_p ((bool)", stdout);
   rtx_reader_ptr->print_c_condition (test->expr);
-  fputs ("\n    ? (int) ", stdout);
+  fputs (")\n    ? (int) (bool)", stdout);
   rtx_reader_ptr->print_c_condition (test->expr);
   fputs ("\n    : -1 },\n", stdout);
   return 1;
diff --git a/gcc/genopinit.cc b/gcc/genopinit.cc
index d8682b2a9ad56a0a62b4407741c695489c72795b..0d9cf0de8b93da5884a352858b343f81644f9d3f 100644
--- a/gcc/genopinit.cc
+++ b/gcc/genopinit.cc
@@ -386,7 +386,7 @@  main (int argc, const char **argv)
 	  unsigned end = MIN (patterns.length (),
 			      (i + 1) * patterns_per_function);
 	  for (j = start; j < end; ++j)
-	    fprintf (s_file, "  ena[%u] = HAVE_%s;\n", j, patterns[j].name);
+	    fprintf (s_file, "  ena[%u] = (bool) HAVE_%s;\n", j, patterns[j].name);
 	  fprintf (s_file, "}\n\n");
 	}
 
@@ -402,7 +402,7 @@  main (int argc, const char **argv)
 	       "(struct target_optabs *optabs)\n{\n");
       fprintf (s_file, "  bool *ena = optabs->pat_enable;\n");
       for (i = 0; patterns.iterate (i, &p); ++i)
-	fprintf (s_file, "  ena[%u] = HAVE_%s;\n", i, p->name);
+	fprintf (s_file, "  ena[%u] = (bool) HAVE_%s;\n", i, p->name);
       fprintf (s_file, "}\n\n");
     }
 
diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
index 8c84deea97d31a844d0c217004d13cc5e95e744d..97117c67e736211d86a231ac046f51a5e9459c43 100644
--- a/gcc/targhooks.cc
+++ b/gcc/targhooks.cc
@@ -620,7 +620,7 @@  bool
 default_float_exceptions_rounding_supported_p (void)
 {
 #ifdef HAVE_adddf3
-  return HAVE_adddf3;
+  return (bool) HAVE_adddf3;
 #else
   return false;
 #endif