On Tue, 2020-12-01 at 15:34 -0700, Jeff Law wrote:
>
> No strong opinions. I think whichever is less invasive in terms of
> code
> quality is probably the way to go. What we want to avoid is
> suppressing
> threading unnecessarily as that often leads to false positives from
> middle-end based warnings. Suppressing threading can also lead to
> build
> failures in the kernel due to the way they use b_c_p.
I think v1 is better then. Would you mind approving the following?
That's the same code as in v1, but with the improved commit message and
comments.
Linux Kernel (specifically, drivers/leds/trigger/ledtrig-cpu.c) build
with GCC 10 fails on s390 with "impossible constraint".
Explanation by Jeff Law:
```
So what we have is a b_c_p at the start of an if-else chain. Subsequent
tests on the "true" arm of the the b_c_p test may throw us off the
constant path (because the constants are out of range). Once all the
tests are passed (it's constant and the constant is in range) the true
arm's terminal block has a special asm that requires a constant
argument. In the case where we get to the terminal block on the true
arm, the argument to the b_c_p is used as the constant argument to the
special asm.
At first glace jump threading seems to be doing the right thing. Except
that we end up with two paths to that terminal block with the special
asm, one for each of the two constant arguments to the b_c_p call.
Naturally since that same value is used in the asm, we have to introduce
a PHI to select between them at the head of the terminal block. Now
the argument in the asm is no longer constant and boom we fail.
```
Fix by disallowing __builtin_constant_p on threading paths.
gcc/ChangeLog:
2020-06-03 Ilya Leoshkevich <i...@linux.ibm.com>
* tree-ssa-threadbackward.c (thread_jumps::profitable_jump_thread_path):
Do not allow __builtin_constant_p on a threading path.
gcc/testsuite/ChangeLog:
2020-06-03 Ilya Leoshkevich <i...@linux.ibm.com>
* gcc.target/s390/builtin-constant-p-threading.c: New test.
---
.../s390/builtin-constant-p-threading.c | 46 +++++++++++++++++++
gcc/tree-ssa-threadbackward.c | 7 ++-
2 files changed, 52 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/s390/builtin-constant-p-threading.c
diff --git a/gcc/testsuite/gcc.target/s390/builtin-constant-p-threading.c
b/gcc/testsuite/gcc.target/s390/builtin-constant-p-threading.c
new file mode 100644
index 00000000000..5f0acdce0b0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/builtin-constant-p-threading.c
@@ -0,0 +1,46 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=z196 -mzarch" } */
+
+typedef struct
+{
+ int counter;
+} atomic_t;
+
+static inline __attribute__ ((__gnu_inline__)) int
+__atomic_add (int val, int *ptr)
+{
+ int old;
+ asm volatile("laa %[old],%[val],%[ptr]\n"
+ : [old] "=d" (old), [ptr] "+Q"(*ptr)
+ : [val] "d" (val)
+ : "cc", "memory");
+ return old;
+}
+
+static inline __attribute__ ((__gnu_inline__)) void
+__atomic_add_const (int val, int *ptr)
+{
+ asm volatile("asi %[ptr],%[val]\n"
+ : [ptr] "+Q" (*ptr)
+ : [val] "i" (val)
+ : "cc", "memory");
+}
+
+static inline __attribute__ ((__gnu_inline__)) void
+atomic_add (int i, atomic_t *v)
+{
+ if (__builtin_constant_p (i) && (i > -129) && (i < 128))
+ {
+ __atomic_add_const (i, &v->counter);
+ return;
+ }
+ __atomic_add (i, &v->counter);
+}
+
+static atomic_t num_active_cpus = { (0) };
+
+void
+ledtrig_cpu (_Bool is_active)
+{
+ atomic_add (is_active ? 1 : -1, &num_active_cpus);
+}
diff --git a/gcc/tree-ssa-threadbackward.c b/gcc/tree-ssa-threadbackward.c
index 327628f1662..30f692672d9 100644
--- a/gcc/tree-ssa-threadbackward.c
+++ b/gcc/tree-ssa-threadbackward.c
@@ -259,8 +259,13 @@ thread_jumps::profitable_jump_thread_path (basic_block
bbi, tree name,
!gsi_end_p (gsi);
gsi_next_nondebug (&gsi))
{
+ /* Do not allow OpenACC loop markers and __builtin_constant_p on
+ threading paths. The latter is disallowed, because an
+ expression might be constant on two threading paths, and
+ become non-constant (i.e.: phi) when they merge. */
gimple *stmt = gsi_stmt (gsi);
- if (gimple_call_internal_p (stmt, IFN_UNIQUE))
+ if (gimple_call_internal_p (stmt, IFN_UNIQUE)
+ || gimple_call_builtin_p (stmt, BUILT_IN_CONSTANT_P))
{
m_path.pop ();
return NULL;
--
2.25.4
