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

[Peter Bergner <bergner@linux.ibm.com>]

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