[LTO,ARM] Fix vector TYPE_MODE in streaming-out

Hello,

This patch fixes a few ICEs that I'm having while enabling the ARM/NEON 
builtins for LTO (in a parallel development line, so the testcase is 
cannot work in isolation but should illustrate the problem)

For instance, the global declarations in the following code compiled 
with -mfpu=neon

----------------------------------
__simd64_int8_t a;
__simd128_int16_t e;

int
main()
{
    e = __builtin_neon_vaddlsv8qi (a, a);
    return 0;
}
----------------------------------


in "normal" mode, the TYPE_MODE for vector_type __simd64_int8_t is set 
to V8QImode by arm_vector_mode_supported_p during the builtins type 
initializations, thanks to TARGET_NEON set bu the global flag.

Now, in LTO mode the streamer writes the information for this 
vector_type as a scalar DImode, causing ICEs during arm_expand_builtin. 
The root cause of this is that the streamer-out uses TYPE_MODE in a 
context where the target_flags are not known return false for TARGET_NEON.

The streamer-in then will then read the wrong mode that propagates to 
the back-end.

Using the value computed by the middle end (when the current target was 
known) in type_common.mode instead of calling vector_type_mode during 
the LTO streaming fixes this issue.

Does that seem all-right to skip vector_type_mode dynamic machinery in 
the lto streamer since the streamer should not change modes used by the 
middle-end (I assume) ?

comments ?

many thanks

Christian

(for ref: this is a follow-up of 
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01094.html)

On 12/15/2015 04:09 PM, Christian Bruel wrote:
> in "normal" mode, the TYPE_MODE for vector_type __simd64_int8_t is set
> to V8QImode by arm_vector_mode_supported_p during the builtins type
> initializations, thanks to TARGET_NEON set bu the global flag.
>
> Now, in LTO mode the streamer writes the information for this
> vector_type as a scalar DImode, causing ICEs during arm_expand_builtin.
> The root cause of this is that the streamer-out uses TYPE_MODE in a
> context where the target_flags are not known return false for TARGET_NEON.
>
> The streamer-in then will then read the wrong mode that propagates to
> the back-end.

>  static void
>  pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>  {
> -  bp_pack_machine_mode (bp, TYPE_MODE (expr));
> +  bp_pack_machine_mode (bp, expr->type_common.mode);

This looks sensible given that tree-streamer-in uses SET_TYPE_MODE, 
which just writes expr->type_common.mode.

Make a new macro TYPE_MODE_RAW for this and I think the patch is ok 
(although there's precedent for direct access in vector_type_mode, but I 
think that's just bad).


Bernd

On Tue, Dec 15, 2015 at 5:14 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 12/15/2015 04:09 PM, Christian Bruel wrote:
>>
>> in "normal" mode, the TYPE_MODE for vector_type __simd64_int8_t is set
>> to V8QImode by arm_vector_mode_supported_p during the builtins type
>> initializations, thanks to TARGET_NEON set bu the global flag.
>>
>> Now, in LTO mode the streamer writes the information for this
>> vector_type as a scalar DImode, causing ICEs during arm_expand_builtin.
>> The root cause of this is that the streamer-out uses TYPE_MODE in a
>> context where the target_flags are not known return false for TARGET_NEON.
>>
>> The streamer-in then will then read the wrong mode that propagates to
>> the back-end.
>
>
>>  static void
>>  pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>  {
>> -  bp_pack_machine_mode (bp, TYPE_MODE (expr));
>> +  bp_pack_machine_mode (bp, expr->type_common.mode);
>
>
> This looks sensible given that tree-streamer-in uses SET_TYPE_MODE, which
> just writes expr->type_common.mode.
>
> Make a new macro TYPE_MODE_RAW for this and I think the patch is ok
> (although there's precedent for direct access in vector_type_mode, but I
> think that's just bad).

Yeah, though it's well-hidden ;)

I think the patch is ok if you add a comment why we're not using TYPE_MODE here
and if the patch passes the x86_64 vectorizer testsuite with -m32 -march=i586
with no regressions (I do expect some FAILs with -march=i586 but the
patch shouldn't
regress anything).

Thanks,
Richard.


>
> Bernd