[PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

When we expand an MMA disassemble built-in with C++ using a pointer that
is casted to a valid MMA type, the type isn't passed down to the expand
machinery and we end up using the base type of the pointer which leads to
an ICE.  This patch enforces we always use the correct MMA type regardless
of the pointer type being used.

This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
Ok for trunk and backports after some burn-in time?

Peter

gcc/
	PR target/101322
	* config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_mma_builtin):
	Enforce the use of a valid MMA pointer type.

gcc/testsuite/
	PR target/101322
	* g++.target/powerpc/pr101322.C: New test.

diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
index 12afa86854c..e796e74f072 100644
--- a/gcc/config/rs6000/rs6000-builtin.cc
+++ b/gcc/config/rs6000/rs6000-builtin.cc
@@ -1085,7 +1085,12 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi,
       unsigned nvec = (fncode == RS6000_BIF_DISASSEMBLE_ACC) ? 4 : 2;
       tree dst_ptr = gimple_call_arg (stmt, 0);
       tree src_ptr = gimple_call_arg (stmt, 1);
-      tree src_type = TREE_TYPE (src_ptr);
+      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
+		      ? build_pointer_type (vector_quad_type_node)
+		      : build_pointer_type (vector_pair_type_node);
+      if (TREE_TYPE (TREE_TYPE (src_ptr)) != src_type)
+	src_ptr = build1 (VIEW_CONVERT_EXPR, src_type, src_ptr);
+
       tree src = create_tmp_reg_or_ssa_name (TREE_TYPE (src_type));
       gimplify_assign (src, build_simple_mem_ref (src_ptr), &new_seq);
 
diff --git a/gcc/testsuite/g++.target/powerpc/pr101322.C b/gcc/testsuite/g++.target/powerpc/pr101322.C
new file mode 100644
index 00000000000..59e71e8eb89
--- /dev/null
+++ b/gcc/testsuite/g++.target/powerpc/pr101322.C
@@ -0,0 +1,17 @@
+/* PR target/101322 */
+/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
+/* { dg-require-effective-target power10_ok } */
+
+/* Verify we don't ICE on the following test cases.  */
+
+void
+foo (char *resp, char *vpp)
+{
+  __builtin_vsx_disassemble_pair (resp, (__vector_pair *) vpp);
+}
+
+void
+bar (char *resp, char *vpp)
+{
+  __builtin_mma_disassemble_acc (resp, (__vector_quad *)vpp);
+}

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Kewen.Lin]

Hi Peter,

Thanks for the patch!  Some comments are inline as below.

on 2022/8/27 11:50, Peter Bergner via Gcc-patches wrote:
> When we expand an MMA disassemble built-in with C++ using a pointer that
> is casted to a valid MMA type, the type isn't passed down to the expand
> machinery and we end up using the base type of the pointer which leads to
> an ICE.  This patch enforces we always use the correct MMA type regardless
> of the pointer type being used.
> 
> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
> Ok for trunk and backports after some burn-in time?
> 
> Peter
> 
> gcc/
> 	PR target/101322
> 	* config/rs6000/rs6000-builtin.cc (rs6000_gimple_fold_mma_builtin):
> 	Enforce the use of a valid MMA pointer type.
> 
> gcc/testsuite/
> 	PR target/101322
> 	* g++.target/powerpc/pr101322.C: New test.
> 
> diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
> index 12afa86854c..e796e74f072 100644
> --- a/gcc/config/rs6000/rs6000-builtin.cc
> +++ b/gcc/config/rs6000/rs6000-builtin.cc
> @@ -1085,7 +1085,12 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi,
>        unsigned nvec = (fncode == RS6000_BIF_DISASSEMBLE_ACC) ? 4 : 2;
>        tree dst_ptr = gimple_call_arg (stmt, 0);
>        tree src_ptr = gimple_call_arg (stmt, 1);
> -      tree src_type = TREE_TYPE (src_ptr);
> +      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
> +		      ? build_pointer_type (vector_quad_type_node)
> +		      : build_pointer_type (vector_pair_type_node);

Nit: it seems we can use existing ptr_vector_quad_type_node and ptr_vector_pair_type_node?
I assume the const qualifier is fine since it's for disassembling.

> +      if (TREE_TYPE (TREE_TYPE (src_ptr)) != src_type)

This line looks unexpected, the former is type char while the latter is type __vector_pair *.

I guess you meant to compare the type of pointer type like: 
   
   TREE_TYPE (TREE_TYPE (src_ptr)) != TREE_TYPE (src_type)

or even with mode like:

   TYPE_MODE (TREE_TYPE (TREE_TYPE (src_ptr))) != TYPE_MODE (TREE_TYPE (src_type))

> +	src_ptr = build1 (VIEW_CONVERT_EXPR, src_type, src_ptr);

Nit: NOP_EXPR seems to be better suited here for pointer conversion.

BR,
Kewen

> +
>        tree src = create_tmp_reg_or_ssa_name (TREE_TYPE (src_type));
>        gimplify_assign (src, build_simple_mem_ref (src_ptr), &new_seq);
>  
> diff --git a/gcc/testsuite/g++.target/powerpc/pr101322.C b/gcc/testsuite/g++.target/powerpc/pr101322.C
> new file mode 100644
> index 00000000000..59e71e8eb89
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/pr101322.C
> @@ -0,0 +1,17 @@
> +/* PR target/101322 */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +/* { dg-require-effective-target power10_ok } */
> +
> +/* Verify we don't ICE on the following test cases.  */
> +
> +void
> +foo (char *resp, char *vpp)
> +{
> +  __builtin_vsx_disassemble_pair (resp, (__vector_pair *) vpp);
> +}
> +
> +void
> +bar (char *resp, char *vpp)
> +{
> +  __builtin_mma_disassemble_acc (resp, (__vector_quad *)vpp);
> +}

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

On 8/31/22 4:22 AM, Kewen.Lin wrote:
> on 2022/8/27 11:50, Peter Bergner via Gcc-patches wrote:
>> -      tree src_type = TREE_TYPE (src_ptr);
>> +      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
>> +		      ? build_pointer_type (vector_quad_type_node)
>> +		      : build_pointer_type (vector_pair_type_node);
> 
> Nit: it seems we can use existing ptr_vector_quad_type_node and ptr_vector_pair_type_node?
> I assume the const qualifier is fine since it's for disassembling.

That's actually what I started with, but I got some type of gimple
verification error which I can't remember what it said.  Let me put
that back temporarily and I'll grab the error message.



>> +      if (TREE_TYPE (TREE_TYPE (src_ptr)) != src_type)
> 
> This line looks unexpected, the former is type char while the latter is type __vector_pair *.
> 
> I guess you meant to compare the type of pointer type like: 
>    
>    TREE_TYPE (TREE_TYPE (src_ptr)) != TREE_TYPE (src_type)
> 
> or even with mode like:
> 
>    TYPE_MODE (TREE_TYPE (TREE_TYPE (src_ptr))) != TYPE_MODE (TREE_TYPE (src_type))

Maybe?  However, if that is the case, how can it be working for me?
Let me throw this in the debugger and verify the types and I'll report
back with what I find.



>> +	src_ptr = build1 (VIEW_CONVERT_EXPR, src_type, src_ptr);
> 
> Nit: NOP_EXPR seems to be better suited here for pointer conversion.

NOP_EXPR is new to me (I don't play in the middle-end that often).
Let me look around at some other uses of it and I'll give it a try.
Thanks!

Peter

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

On 8/31/22 8:59 AM, Peter Bergner wrote:
> On 8/31/22 4:22 AM, Kewen.Lin wrote:
>> on 2022/8/27 11:50, Peter Bergner via Gcc-patches wrote:
>>> -      tree src_type = TREE_TYPE (src_ptr);
>>> +      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
>>> +		      ? build_pointer_type (vector_quad_type_node)
>>> +		      : build_pointer_type (vector_pair_type_node);
>>
>> Nit: it seems we can use existing ptr_vector_quad_type_node and ptr_vector_pair_type_node?
>> I assume the const qualifier is fine since it's for disassembling.
> 
> That's actually what I started with, but I got some type of gimple
> verification error which I can't remember what it said.  Let me put
> that back temporarily and I'll grab the error message.

...and of course, now I can't recreate that issue at all and the
ptr_vector_*_type use work fine now.  Strange! ...so ok, changed.
Maybe the behavior changed since my PR106017 fix went in???



>>> +      if (TREE_TYPE (TREE_TYPE (src_ptr)) != src_type)
>>
>> This line looks unexpected, the former is type char while the latter is type __vector_pair *.
>>
>> I guess you meant to compare the type of pointer type like: 
>>    
>>    TREE_TYPE (TREE_TYPE (src_ptr)) != TREE_TYPE (src_type)
> 
> Maybe?  However, if that is the case, how can it be working for me?
> Let me throw this in the debugger and verify the types and I'll report
> back with what I find.

Ok, you are correct.  Thanks for catching that!  I don't think we need
those matching outer TREE_TYPE() uses.  I think just a simple:

	if (TREE_TYPE (src_ptr) != src_type)

...should suffice.


>> or even with mode like:
>>
>>    TYPE_MODE (TREE_TYPE (TREE_TYPE (src_ptr))) != TYPE_MODE (TREE_TYPE (src_type))

I'd rather not look at the mode here, since OOmode/XOmode doesn't necessarily
mean __vector_{pair,quad}, so I'll go with the modified test above.




>>> +	src_ptr = build1 (VIEW_CONVERT_EXPR, src_type, src_ptr);
>>
>> Nit: NOP_EXPR seems to be better suited here for pointer conversion.

Ok, this works too, so code changed to use it.  Thanks!

Question for my own education, when would you use VIEW_CONVERT_EXPR over NOP_EXPR?



FYI, here is the current code patch with all of the suggested changes incorporated:

diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
index 12afa86854c..61352fcd801 100644
--- a/gcc/config/rs6000/rs6000-builtin.cc
+++ b/gcc/config/rs6000/rs6000-builtin.cc
@@ -1085,7 +1085,11 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi,
       unsigned nvec = (fncode == RS6000_BIF_DISASSEMBLE_ACC) ? 4 : 2;
       tree dst_ptr = gimple_call_arg (stmt, 0);
       tree src_ptr = gimple_call_arg (stmt, 1);
-      tree src_type = TREE_TYPE (src_ptr);
+      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
+                     ? ptr_vector_quad_type_node : ptr_vector_pair_type_node;
+      if (TREE_TYPE (src_ptr) != src_type)
+       src_ptr = build1 (NOP_EXPR, src_type, src_ptr);
+
       tree src = create_tmp_reg_or_ssa_name (TREE_TYPE (src_type));
       gimplify_assign (src, build_simple_mem_ref (src_ptr), &new_seq);
 

I'll fire off a new round of bootstrap and regtesting and resubmit if it's clean.
Thanks for the review!


Peter

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 01:53:48PM -0500, Peter Bergner wrote:
> ...and of course, now I can't recreate that issue at all and the
> ptr_vector_*_type use work fine now.  Strange! ...so ok, changed.
> Maybe the behavior changed since my PR106017 fix went in???

That is my best guess as well.  But, how did that help this test?

> Question for my own education, when would you use VIEW_CONVERT_EXPR over NOP_EXPR?

VIEW_CONVERT_EXPR is essentially a bit_cast.  Only use it when you need
that, it is sub-optimal if you don't.

> FYI, here is the current code patch with all of the suggested changes incorporated:

> --- a/gcc/config/rs6000/rs6000-builtin.cc
> +++ b/gcc/config/rs6000/rs6000-builtin.cc
> @@ -1085,7 +1085,11 @@ rs6000_gimple_fold_mma_builtin (gimple_stmt_iterator *gsi,
>        unsigned nvec = (fncode == RS6000_BIF_DISASSEMBLE_ACC) ? 4 : 2;
>        tree dst_ptr = gimple_call_arg (stmt, 0);
>        tree src_ptr = gimple_call_arg (stmt, 1);
> -      tree src_type = TREE_TYPE (src_ptr);
> +      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
> +                     ? ptr_vector_quad_type_node : ptr_vector_pair_type_node;

If you split a?b:c over multiple lines, please make it three lines.


Segher

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

On 8/31/22 3:51 PM, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 01:53:48PM -0500, Peter Bergner wrote:
>> Question for my own education, when would you use VIEW_CONVERT_EXPR over NOP_EXPR?
> 
> VIEW_CONVERT_EXPR is essentially a bit_cast.  Only use it when you need
> that, it is sub-optimal if you don't.

Ok.  I believe I added a couple of other similar uses of VIEW_CONVERT_EXPR for
pointer casts on the __builtin_vsx_lxvp/stxvp built-ins.  I'll try converting
those to use NOP_EXPR too in a separate cleanup patch.  Thanks!



>> -      tree src_type = TREE_TYPE (src_ptr);
>> +      tree src_type = (fncode == RS6000_BIF_DISASSEMBLE_ACC)
>> +                     ? ptr_vector_quad_type_node : ptr_vector_pair_type_node;
> 
> If you split a?b:c over multiple lines, please make it three lines.

Can do, however...


>> ...and of course, now I can't recreate that issue at all and the
>> ptr_vector_*_type use work fine now.  Strange! ...so ok, changed.
>> Maybe the behavior changed since my PR106017 fix went in???
> 
> That is my best guess as well.  But, how did that help this test?

It didn't. :-)   During my bootstrap, I hit the gimple verification issue
I mentioned seeing earlier.  My problem was I thought I hit it with the
test case, but it was exposed on a different test case in the testsuite.
Here's what I'm seeing, which only happens when using -O0 -flto:

rain6p1% gcc -O0 -mcpu=power10 -flto pr102347.c 
lto1: internal compiler error: in gimple_canonical_types_compatible_p, at tree.cc:13677
0x11930a97 gimple_canonical_types_compatible_p(tree_node const*, tree_node const*, bool)
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13677
0x1192f1ab verify_type_variant
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13377
0x11930beb verify_type(tree_node const*)
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13700
0x106bbd37 lto_fixup_state
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2629
0x106bbff3 lto_fixup_decls
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2660
0x106bce13 read_cgraph_and_symbols(unsigned int, char const**)
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2901
0x1067bcbf lto_main()
	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto.cc:656
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
lto-wrapper: fatal error: /home/bergner/gcc/build/gcc-fsf-mainline-pr101322-debug/gcc/xgcc returned 1 exit status
compilation terminated.
/home/bergner/binutils/install/binutils-power10/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status

The problem goes away if I use use -O1 or above, I drop -flto or I use
the code I originally posted without the ptr_vector_*_type

The assert in gimple_canonical_types_compatible_p() we're hitting is:
13673	    default:
13674	      /* Consider all types with language specific trees in them mutually
13675		 compatible.  This is executed only from verify_type and false
13676	         positives can be tolerated.  */
13677	      gcc_assert (!in_lto_p);
13678	      return true;

I have no idea why ptr_vector_*_type would behave differently here than
build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
in lto???

Peter

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Segher Boessenkool]

On Wed, Aug 31, 2022 at 05:01:04PM -0500, Peter Bergner wrote:
> The problem goes away if I use use -O1 or above, I drop -flto or I use
> the code I originally posted without the ptr_vector_*_type
> 
> The assert in gimple_canonical_types_compatible_p() we're hitting is:
> 13673	    default:
> 13674	      /* Consider all types with language specific trees in them mutually
> 13675		 compatible.  This is executed only from verify_type and false
> 13676	         positives can be tolerated.  */
> 13677	      gcc_assert (!in_lto_p);
> 13678	      return true;
> 
> I have no idea why ptr_vector_*_type would behave differently here than
> build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
> fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
> in lto???

It looks like left-over debugging code.  Or this truly should never
happen in LTO?


Segher

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

On 8/31/22 6:08 PM, Segher Boessenkool wrote:
> On Wed, Aug 31, 2022 at 05:01:04PM -0500, Peter Bergner wrote:
>> The problem goes away if I use use -O1 or above, I drop -flto or I use
>> the code I originally posted without the ptr_vector_*_type
>>
>> The assert in gimple_canonical_types_compatible_p() we're hitting is:
>> 13673	    default:
>> 13674	      /* Consider all types with language specific trees in them mutually
>> 13675		 compatible.  This is executed only from verify_type and false
>> 13676	         positives can be tolerated.  */
>> 13677	      gcc_assert (!in_lto_p);
>> 13678	      return true;
>>
>> I have no idea why ptr_vector_*_type would behave differently here than
>> build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
>> fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
>> in lto???
> 
> It looks like left-over debugging code.  Or this truly should never
> happen in LTO?

I have no idea.  Either way, I'm going to go with the code that works,
because either it's the "correct" code or it works around buggy code
in lto.  Either is a valid reason to me to use it.

Peter

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Kewen.Lin]

>>> ...and of course, now I can't recreate that issue at all and the
>>> ptr_vector_*_type use work fine now.  Strange! ...so ok, changed.
>>> Maybe the behavior changed since my PR106017 fix went in???
>>
>> That is my best guess as well.  But, how did that help this test?
> 
> It didn't. :-)   During my bootstrap, I hit the gimple verification issue
> I mentioned seeing earlier.  My problem was I thought I hit it with the
> test case, but it was exposed on a different test case in the testsuite.
> Here's what I'm seeing, which only happens when using -O0 -flto:
> 
> rain6p1% gcc -O0 -mcpu=power10 -flto pr102347.c 
> lto1: internal compiler error: in gimple_canonical_types_compatible_p, at tree.cc:13677
> 0x11930a97 gimple_canonical_types_compatible_p(tree_node const*, tree_node const*, bool)
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13677
> 0x1192f1ab verify_type_variant
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13377
> 0x11930beb verify_type(tree_node const*)
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/tree.cc:13700
> 0x106bbd37 lto_fixup_state
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2629
> 0x106bbff3 lto_fixup_decls
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2660
> 0x106bce13 read_cgraph_and_symbols(unsigned int, char const**)
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto-common.cc:2901
> 0x1067bcbf lto_main()
> 	/home/bergner/gcc/gcc-fsf-mainline-pr101322/gcc/lto/lto.cc:656
> Please submit a full bug report, with preprocessed source (by using -freport-bug).
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
> lto-wrapper: fatal error: /home/bergner/gcc/build/gcc-fsf-mainline-pr101322-debug/gcc/xgcc returned 1 exit status
> compilation terminated.
> /home/bergner/binutils/install/binutils-power10/bin/ld: error: lto-wrapper failed
> collect2: error: ld returned 1 exit status
> 
> The problem goes away if I use use -O1 or above, I drop -flto or I use
> the code I originally posted without the ptr_vector_*_type
> 
> The assert in gimple_canonical_types_compatible_p() we're hitting is:
> 13673	    default:
> 13674	      /* Consider all types with language specific trees in them mutually
> 13675		 compatible.  This is executed only from verify_type and false
> 13676	         positives can be tolerated.  */
> 13677	      gcc_assert (!in_lto_p);
> 13678	      return true;
> 
> I have no idea why ptr_vector_*_type would behave differently here than
> build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
> fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
> in lto???

Thanks for your time to reproduce this!

The only difference is that ptr_vector_*_type are built from the
qualified_type based on vector_*_type_node, instead of directly from
vector_*_type_node.  I'm interested to have a further look at this later.

BR,
Kewen

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Peter Bergner]

On 9/1/22 3:29 AM, Kewen.Lin wrote:
>> I have no idea why ptr_vector_*_type would behave differently here than
>> build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
>> fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
>> in lto???
> 
> Thanks for your time to reproduce this!
> 
> The only difference is that ptr_vector_*_type are built from the
> qualified_type based on vector_*_type_node, instead of directly from
> vector_*_type_node.  I'm interested to have a further look at this later.

If you look into this, please let me know.  I'd like to know what you
find out.

Peter

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Kewen.Lin]

on 2022/9/1 22:17, Peter Bergner wrote:
> On 9/1/22 3:29 AM, Kewen.Lin wrote:
>>> I have no idea why ptr_vector_*_type would behave differently here than
>>> build_pointer_type (vector_*_type_node).  Using the build_pointer_type()
>>> fixed it for me, so that's why I went with it. :-)  Maybe this is a bug
>>> in lto???
>>
>> Thanks for your time to reproduce this!
>>
>> The only difference is that ptr_vector_*_type are built from the
>> qualified_type based on vector_*_type_node, instead of directly from
>> vector_*_type_node.  I'm interested to have a further look at this later.
> 
> If you look into this, please let me know.  I'd like to know what you
> find out.

I just filed PR106833 for it.

BR,
Kewen

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Kewen.Lin]

>>>> +      if (TREE_TYPE (TREE_TYPE (src_ptr)) != src_type)
>>>
>>> This line looks unexpected, the former is type char while the latter is type __vector_pair *.
>>>
>>> I guess you meant to compare the type of pointer type like: 
>>>    
>>>    TREE_TYPE (TREE_TYPE (src_ptr)) != TREE_TYPE (src_type)
>>
>> Maybe?  However, if that is the case, how can it be working for me?
>> Let me throw this in the debugger and verify the types and I'll report
>> back with what I find.
> 
> Ok, you are correct.  Thanks for catching that!  I don't think we need
> those matching outer TREE_TYPE() uses.  I think just a simple:
> 
> 	if (TREE_TYPE (src_ptr) != src_type)
> 
> ...should suffice.
> 

Yeah, it's enough for the associated test case.  :)

> 
>>> or even with mode like:
>>>
>>>    TYPE_MODE (TREE_TYPE (TREE_TYPE (src_ptr))) != TYPE_MODE (TREE_TYPE (src_type))
> 
> I'd rather not look at the mode here, since OOmode/XOmode doesn't necessarily
> mean __vector_{pair,quad}, so I'll go with the modified test above.

Good point.  I thought the cv qualifier can affect the type equality check and
assumed for test case like:

void
foo (char *resp, const __vector_pair *vpp)
{
  __builtin_vsx_disassemble_pair (resp, (__vector_pair *) vpp);
}

, we don't want to have the conversion there and the ICE seems related to the
underlying mode, so I thought maybe you wanted to use TYPE_MODE.


>>>> +	src_ptr = build1 (VIEW_CONVERT_EXPR, src_type, src_ptr);
>>>
>>> Nit: NOP_EXPR seems to be better suited here for pointer conversion.
> 
> Ok, this works too, so code changed to use it.  Thanks!
> 
> Question for my own education, when would you use VIEW_CONVERT_EXPR over NOP_EXPR?

tree.def has some note about VIEW_CONVERT_EXPR, it quite matches what Segher replied.
In my experience, VIEW_CONVERT_EXPR are used a lot for vector type conversion.

BR,
Kewen

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Segher Boessenkool]

On Thu, Sep 01, 2022 at 04:28:56PM +0800, Kewen.Lin wrote:
> tree.def has some note about VIEW_CONVERT_EXPR, it quite matches what Segher replied.
> In my experience, VIEW_CONVERT_EXPR are used a lot for vector type conversion.

It is needed whenever vector types are not compatible otherwise.
V4SI <-> V4SF, V4SI <-> V2DI, etc.  In such cases you effectively do a
bit_cast equivalent.


Segher

Re: [PATCH] rs6000: Don't ICE when we disassemble an MMA variable [PR101322]

[Segher Boessenkool]

Hi!

On Fri, Aug 26, 2022 at 10:50:00PM -0500, Peter Bergner wrote:
> When we expand an MMA disassemble built-in with C++ using a pointer that
> is casted to a valid MMA type, the type isn't passed down to the expand

(The perfect tense of cast is "cast").

> machinery and we end up using the base type of the pointer which leads to
> an ICE.  This patch enforces we always use the correct MMA type regardless
> of the pointer type being used.

> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/pr101322.C
> @@ -0,0 +1,17 @@
> +/* PR target/101322 */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +/* { dg-require-effective-target power10_ok } */

These should be the other way around.  It makes no sense to set option X
before testing if you can set option X.  Even if it would technically
work, the primary consumer of any source code is the humans who read it,
and humans read code top down.


Segher