This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, rs6000] Fix PR83399, ICE During LRA with 2-op rtl pattern for lvx instruction


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]