On Tue, Jan 09, 2018 at 12:22:38PM -0600, Peter Bergner wrote:
> PR83399 shows a problem where we emit an altivec load using a builtin
> that forces us to use a specific altivec load pattern. The generated
> rtl pattern has a use of sfp (frame pointer) and during LRA, we eliminate
> it's use to the sp (lra-eliminations.c:process_insn_for_elimination).
> During this process, we re-recog the insn and end up matching a different
> vsx pattern, because it exists earlier in the machine description file.
> That vsx pattern uses a "Z" constraint for its address operand, which will
> not accept the "special" altivec address we have, but the memory_operand
> predicate the pattern uses allows it. The recog'ing to a different pattern
> than we want, causes us to ICE later on.
>
> The solution here is to tighten the predicate used for the address in the
> vsx pattern to use the indexed_or_indirect_operand instead, which will
> reject the altivec address our rtl pattern has.
>
> Once this is fixed, we end up hitting another issue in print_operand when
> outputing altivec addresses when using -mvsx. This was fixed by allowing
> both ALTIVEC or VSX VECTOR MEMs.
>
> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
> Ok for trunk?
>
> Is this ok for the open release branches too, once testing has completed
> there?
>
> Peter
>
>
> gcc/
> PR target/83399
> * config/rs6000/rs6000.c (print_operand): Use
> VECTOR_MEM_ALTIVEC_OR_VSX_P.
(print_operand) <'y'>: ...
> * config/rs6000/vsx.md (*vsx_le_perm_load_<mode><VSX_D>): Use
> indexed_or_indirect_operand predicate.
It's *vsx_le_perm_load_<mode> -- <VSX_D> is not part of the name. It
makes sense to mention it, maybe like
* config/rs6000/vsx.md (*vsx_le_perm_load_<mode> for VSX_D): ...
> (*vsx_le_perm_load_<mode><VSX_W>): Likewise.
Similar.
> (*vsx_le_perm_load_v8hi): Likewise.
> (*vsx_le_perm_load_v8hi): Likewise.
You duplicated this line.
> (*vsx_le_perm_load_v16qi): Likewise.
> (*vsx_le_perm_store_<mode><VSX_D>): Likewise in pattern and splitters.
> (*vsx_le_perm_store_<mode><VSX_W>): Likewise.
Same for these two.
> (*vsx_le_perm_store_v8hi): Likewise.
> (*vsx_le_perm_store_v16qi): Likewise.
>
> gcc/testsuite/
> PR target/83399
> * gcc.target/powerpc/pr83399.c: New test.
> (define_split
> - [(set (match_operand:VSX_D 0 "memory_operand" "")
> + [(set (match_operand:VSX_D 0 "indexed_or_indirect_operand" "")
> (match_operand:VSX_D 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && !reload_completed"
> [(set (match_dup 2)
> @@ -570,7 +570,7 @@ (define_split
> ;; The post-reload split requires that we re-permute the source
> ;; register in case it is still live.
> (define_split
> - [(set (match_operand:VSX_D 0 "memory_operand" "")
> + [(set (match_operand:VSX_D 0 "indexed_or_indirect_operand" "")
> (match_operand:VSX_D 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && reload_completed"
> [(set (match_dup 1)
You don't mention these in the changelog.
> (define_split
> - [(set (match_operand:VSX_W 0 "memory_operand" "")
> + [(set (match_operand:VSX_W 0 "indexed_or_indirect_operand" "")
> (match_operand:VSX_W 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && !reload_completed"
> [(set (match_dup 2)
> @@ -617,7 +617,7 @@ (define_split
> ;; The post-reload split requires that we re-permute the source
> ;; register in case it is still live.
> (define_split
> - [(set (match_operand:VSX_W 0 "memory_operand" "")
> + [(set (match_operand:VSX_W 0 "indexed_or_indirect_operand" "")
> (match_operand:VSX_W 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && reload_completed"
> [(set (match_dup 1)
> @@ -638,7 +638,7 @@ (define_split
> "")
Or these.
> (define_split
> - [(set (match_operand:V8HI 0 "memory_operand" "")
> + [(set (match_operand:V8HI 0 "indexed_or_indirect_operand" "")
> (match_operand:V8HI 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && !reload_completed"
> [(set (match_dup 2)
> @@ -671,7 +671,7 @@ (define_split
> ;; The post-reload split requires that we re-permute the source
> ;; register in case it is still live.
> (define_split
> - [(set (match_operand:V8HI 0 "memory_operand" "")
> + [(set (match_operand:V8HI 0 "indexed_or_indirect_operand" "")
> (match_operand:V8HI 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && reload_completed"
> [(set (match_dup 1)
Or these.
> (define_split
> - [(set (match_operand:V16QI 0 "memory_operand" "")
> + [(set (match_operand:V16QI 0 "indexed_or_indirect_operand" "")
> (match_operand:V16QI 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && !reload_completed"
> [(set (match_dup 2)
> @@ -739,7 +739,7 @@ (define_split
> ;; The post-reload split requires that we re-permute the source
> ;; register in case it is still live.
> (define_split
> - [(set (match_operand:V16QI 0 "memory_operand" "")
> + [(set (match_operand:V16QI 0 "indexed_or_indirect_operand" "")
> (match_operand:V16QI 1 "vsx_register_operand" ""))]
> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR && reload_completed"
> [(set (match_dup 1)
And more :-)
Please fix up the changelog. The patch is fine :-) Okay for trunk, and
okay for the branches after the usual wait. Thanks!
Segher
