Hi Segher!
on 2022/6/25 00:49, Segher Boessenkool wrote:
> Hi!
>
> On Fri, Jun 24, 2022 at 09:03:59AM +0800, Kewen.Lin wrote:
>> on 2022/6/24 03:06, Segher Boessenkool wrote:
>>> On Wed, May 18, 2022 at 10:07:48PM +0800, Kewen.Lin wrote:
>>>> As PR103353 shows, we may want to continue to expand a MMA built-in
>>>> function like a normal function, even if we have already emitted
>>>> error messages about some missing required conditions. As shown in
>>>> that PR, without one explicit mov optab on OOmode provided, it would
>>>> call emit_move_insn recursively.
>>>
>>> First off: lxvp is a VSX insn, not an MMA insn. So please don't call it
>>> that -- this confusion is what presumably caused the problem here, so it
>>> would be good to root it out :-)
>>
>> I guess the "it" in "don't call it call" is for "MMA built-in function"?
>> It comes from the current code:
>
> Your proposed commit message says "MMA built-in function". It is not
> an MMA builtin, or rather, it should not be.
>
>>>> + /* Opaque modes are only expected to be available when MMA is supported,
>>>
>>> Why do people expect that? It is completely wrong. The name "opaque"
>>> itself already says this is not just for MMA, but perhaps more
>>> importantly, it is a basic VSX insn, doesn't touch any MMA resources,
>>> and is useful in other contexts as well.
>>
>> ... The above statements are also based on current code, for now, the
>> related things like built-in functions, mov optab, hard_regno_ok etc.
>> for these two modes are guarded by TARGET_MMA.
>
> Opaque modes are a generic thing, not an rs6000 thing. It is important
> not to conflate completely different things that just happened to
> coincide some months ago (but not anymore right now even!)
>
>> I think I get your points here, you want to separate these opaque
>> modes from MMA since the underlying lxvp/stxvp are not MMA specific,
>> so those related things (bifs, mov optabs etc.) are not necessarily
>> guarded under MMA.
>
> Yup. This can take some time of course, but in the mean time we should
> stop pretending the status quo is correct.
>
>>> So this needs some bigger surgery.
>>
>> Yes, Peter may have more comments on this.
>
> Yes. Can you do a patch that just fixes this PR103353, without adding
> more misleading comments? :-)
>
Many thanks for all the further explanation above! The attached patch
updated the misleading comments as you pointed out and suggested, could
you help to have another look?
BR,
Kewen
From ee49cd14b69aaa373b0aca71c4560944a0b43fbc Mon Sep 17 00:00:00 2001
From: "Kewen.Lin" <li...@gcc.gnu.org>
Date: Mon, 27 Jun 2022 10:42:37 +0800
Subject: [PATCH] rs6000: Adjust mov optabs for opaque modes [PR103353]
As PR103353 shows, we may want to continue to expand built-in
function __builtin_vsx_lxvp, even if we have already emitted
error messages about some missing required conditions. As
shown in that PR, without one explicit mov optab on OOmode
provided, it would call emit_move_insn recursively.
So this patch is to allow the mov pattern to be generated during
expanding phase if compiler has already seen errors.
PR target/103353
gcc/ChangeLog:
* config/rs6000/mma.md (define_expand movoo): Move TARGET_MMA condition
check to preparation statements and add handlings for !TARGET_MMA.
(define_expand movxo): Likewise.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/pr103353.c: New test.
---
gcc/config/rs6000/mma.md | 35 +++++++++++++++++----
gcc/testsuite/gcc.target/powerpc/pr103353.c | 22 +++++++++++++
2 files changed, 51 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103353.c
diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md
index a183b6a168a..a9cf59d68b5 100644
--- a/gcc/config/rs6000/mma.md
+++ b/gcc/config/rs6000/mma.md
@@ -268,10 +268,23 @@ (define_int_attr avvi4i4i4
[(UNSPEC_MMA_PMXVI8GER4PP "pmxvi8ger4pp")
(define_expand "movoo"
[(set (match_operand:OO 0 "nonimmediate_operand")
(match_operand:OO 1 "input_operand"))]
- "TARGET_MMA"
+ ""
{
- rs6000_emit_move (operands[0], operands[1], OOmode);
- DONE;
+ if (TARGET_MMA) {
+ rs6000_emit_move (operands[0], operands[1], OOmode);
+ DONE;
+ }
+ /* PR103353 shows we may want to continue to expand the __builtin_vsx_lxvp
+ built-in function, even if we have already emitted error messages about
+ some missing required conditions. As shown in that PR, without one
+ explicit mov optab on OOmode provided, it would call emit_move_insn
+ recursively. So we allow this pattern to be generated when we are
+ expanding to RTL and have seen errors. It would not cause further ICEs
+ as the compilation would stop soon after expanding. */
+ else if (currently_expanding_to_rtl && seen_error ())
+ ;
+ else
+ gcc_unreachable ();
})
(define_insn_and_split "*movoo"
@@ -300,10 +313,20 @@ (define_insn_and_split "*movoo"
(define_expand "movxo"
[(set (match_operand:XO 0 "nonimmediate_operand")
(match_operand:XO 1 "input_operand"))]
- "TARGET_MMA"
+ ""
{
- rs6000_emit_move (operands[0], operands[1], XOmode);
- DONE;
+ if (TARGET_MMA) {
+ rs6000_emit_move (operands[0], operands[1], XOmode);
+ DONE;
+ }
+ /* PR103353 shows we may want to continue to expand the __builtin_vsx_lxvp
+ built-in function, even if we have already emitted error messages about
+ some missing required conditions. So do the same handlings for XOmode
+ as OOmode here. */
+ else if (currently_expanding_to_rtl && seen_error ())
+ ;
+ else
+ gcc_unreachable ();
})
(define_insn_and_split "*movxo"
diff --git a/gcc/testsuite/gcc.target/powerpc/pr103353.c
b/gcc/testsuite/gcc.target/powerpc/pr103353.c
new file mode 100644
index 00000000000..5d519fb1b7b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr103353.c
@@ -0,0 +1,22 @@
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* If the default cpu type is power10 or later, MMA is enabled by default.
+ To keep the test point available all the time, this case specifies
+ -mdejagnu-cpu=power6 to make it be tested without MMA. */
+/* { dg-options "-maltivec -mdejagnu-cpu=power6" } */
+
+/* Verify there is no ICE and don't check the error messages on MMA
+ requirement since they could be fragile and are not test points
+ of this case. */
+/* { dg-excess-errors "pr103353" } */
+
+void
+foo (__vector_pair *dst, double *x)
+{
+ dst[0] = __builtin_vsx_lxvp (0, (__vector_pair *)(void *)x);
+}
+
+void
+bar (__vector_pair *src, double *x)
+{
+ __builtin_vsx_stxvp (src[0], 0, (__vector_pair *)(void *)x);
+}
--
2.32.0
