[AArch64] Fix predicate and constraint mismatch in logical atomic operations

Discussion:

Michael Collison

2014-09-25 03:45:51 UTC

On certain patterns in atomics.md the constraint 'n' is used in
combination with the predicate atomic_op_operand. The constraint is too
general and allows constants that are disallowed by the predicate. This
causes an ICE In final_scan_insn when the insn cannot be split because
the constraint and predicate do not match.

Tested on aarch64-none-elf, aarch64-linux-gnu. Additionally the
originally reporter of the bug, (***@ubuntu.com), applied the patch and
successfully bootstrapped and tested with no regressions.

2014-09-23 Michael Collison <***@linaro.org>

* config/aarch64/iterators.md (lconst_atomic): New mode attribute to
support constraints for CONST_INT in atomic operations.
* config/aarch64/atomics.md
(atomic_<atomic_optab><mode>): Use lconst_atomic constraint.
(atomic_nand<mode>): Likewise.
(atomic_fetch_<atomic_optab><mode>): Likewise.
(atomic_fetch_nand<mode>): Likewise.
(atomic_<atomic_optab>_fetch<mode>): Likewise.
(atomic_nand_fetch<mode>): Likewise.

--
Michael Collison
Linaro Toolchain Working Group
***@linaro.org

Andrew Pinski

2014-09-25 04:01:07 UTC

Permalink

On Wed, Sep 24, 2014 at 8:45 PM, Michael Collison

Testcase?

* config/aarch64/iterators.md (lconst_atomic): New mode attribute to
support constraints for CONST_INT in atomic operations.
* config/aarch64/atomics.md
(atomic_<atomic_optab><mode>): Use lconst_atomic constraint.
(atomic_nand<mode>): Likewise.
(atomic_fetch_<atomic_optab><mode>): Likewise.
(atomic_fetch_nand<mode>): Likewise.
(atomic_<atomic_optab>_fetch<mode>): Likewise.
(atomic_nand_fetch<mode>): Likewise.
--
Michael Collison
Linaro Toolchain Working Group

Michael Collison

2014-09-25 04:08:49 UTC

Permalink

Testcase attaached. Reproduce with "g++ -c -O2 -fPIE". URL for more
detailed description:

https://bugs.linaro.org/show_bug.cgi?id=331

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 8:45 PM, Michael Collison

Testcase?

--
Michael Collison
Linaro Toolchain Working Group
***@linaro.org

Andrew Pinski

2014-09-25 04:10:41 UTC

Permalink

On Wed, Sep 24, 2014 at 9:08 PM, Michael Collison

Testcase attaached. Reproduce with "g++ -c -O2 -fPIE". URL for more detailed
https://bugs.linaro.org/show_bug.cgi?id=331

It would be a good idea to get a reduced testcase that you could add
to the GCC testsuite so this won't show up again.

Thanks,
Andrew Pinski

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 8:45 PM, Michael Collison

Testcase?

--
Michael Collison
Linaro Toolchain Working Group

Michael Collison

2014-09-25 04:13:11 UTC

Permalink

I have that attached to the bug report at the URL provided. I will work
on a testcase if you think it is warranted.

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 9:08 PM, Michael Collison

Testcase attaached. Reproduce with "g++ -c -O2 -fPIE". URL for more detailed
https://bugs.linaro.org/show_bug.cgi?id=331

It would be a good idea to get a reduced testcase that you could add
to the GCC testsuite so this won't show up again.
Thanks,
Andrew Pinski

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 8:45 PM, Michael Collison

Testcase?

--
Michael Collison
Linaro Toolchain Working Group

--
Michael Collison
Linaro Toolchain Working Group
***@linaro.org

Andrew Pinski

2014-09-25 04:17:23 UTC

Permalink

On Wed, Sep 24, 2014 at 9:13 PM, Michael Collison

I have that attached to the bug report at the URL provided. I will work on a
testcase if you think it is warranted.

Yes it is almost always warranted.

https://gcc.gnu.org/contribute.html#patches

Testcases If you cannot follow the recommendations of the GCC coding
conventions about testcases, you should include a justification for
why adequate testcases cannot be added.

See the last part of that sentence. You don't have any justification
on why you are not including testcases.

-- Pinski

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 9:08 PM, Michael Collison

Testcase attaached. Reproduce with "g++ -c -O2 -fPIE". URL for more detailed
https://bugs.linaro.org/show_bug.cgi?id=331

It would be a good idea to get a reduced testcase that you could add
to the GCC testsuite so this won't show up again.
Thanks,
Andrew Pinski

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 8:45 PM, Michael Collison

Testcase?

--
Michael Collison
Linaro Toolchain Working Group

Segher Boessenkool

2014-09-25 10:12:34 UTC

Permalink

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 9:13 PM, Michael Collison

I have that attached to the bug report at the URL provided. I will work on a
testcase if you think it is warranted.

Yes it is almost always warranted.
https://gcc.gnu.org/contribute.html#patches
Testcases If you cannot follow the recommendations of the GCC coding
conventions about testcases, you should include a justification for
why adequate testcases cannot be added.
See the last part of that sentence. You don't have any justification
on why you are not including testcases.

It is very hard to make a reliable testcase for such problems, because
they only happen when register allocation is under pressure.

The problem is not that "n" allows more than your predicate does. The
predicate allows registers too, so the compiler happily made a register
contain some big const. Now RA comes along, is out of registers but hey,
there is this "n", let's just put the big constant there! Carnage.

So this is hard to test for; you can add some (big) code that exposed the
problem, but in a few months time that won't trigger the problem anymore
because earlier stages in the compiler will have generated slightly
different code.

It also does nothing to catch similar problems in other patterns.

Segher

Michael Collison

2014-09-25 17:33:17 UTC

Permalink

Segher,

The problem is the "CONST_INT 0", not a large constant. This constant is
not accepted by the predicate, but is accepted by the constraint.

Post by Segher Boessenkool

Post by Andrew Pinski
On Wed, Sep 24, 2014 at 9:13 PM, Michael Collison

I have that attached to the bug report at the URL provided. I will work on a
testcase if you think it is warranted.

Yes it is almost always warranted.
https://gcc.gnu.org/contribute.html#patches
Testcases If you cannot follow the recommendations of the GCC coding
conventions about testcases, you should include a justification for
why adequate testcases cannot be added.
See the last part of that sentence. You don't have any justification
on why you are not including testcases.

It is very hard to make a reliable testcase for such problems, because
they only happen when register allocation is under pressure.
The problem is not that "n" allows more than your predicate does. The
predicate allows registers too, so the compiler happily made a register
contain some big const. Now RA comes along, is out of registers but hey,
there is this "n", let's just put the big constant there! Carnage.
So this is hard to test for; you can add some (big) code that exposed the
problem, but in a few months time that won't trigger the problem anymore
because earlier stages in the compiler will have generated slightly
different code.
It also does nothing to catch similar problems in other patterns.
Segher

--
Michael Collison
Linaro Toolchain Working Group
***@linaro.org

Segher Boessenkool

2014-09-25 19:30:48 UTC

Permalink

Post by Michael Collison
The problem is the "CONST_INT 0", not a large constant. This constant is
not accepted by the predicate, but is accepted by the constraint.

Yes, bad choice of words, sorry. Just read "big" as "not matching the
predicate". The point is that everything works fine until RA, and that
makes it hard to make a useful test.

Segher

p.s. Please don't top-post. Thanks.

Christophe Lyon

2014-10-09 12:20:27 UTC

Permalink

On 25 September 2014 21:30, Segher Boessenkool

Post by Segher Boessenkool

Post by Michael Collison
The problem is the "CONST_INT 0", not a large constant. This constant is
not accepted by the predicate, but is accepted by the constraint.

Yes, bad choice of words, sorry. Just read "big" as "not matching the
predicate". The point is that everything works fine until RA, and that
makes it hard to make a useful test.

Hi,

IIUC, a testcase is not required/practicable.
So, may I ping this patch?
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02209.html

Thanks,

Christophe.

Post by Segher Boessenkool
Segher
p.s. Please don't top-post. Thanks.