| Message ID | cb1696dc-5f88-fb46-b8f9-3f83e781b611@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [nvptx] Fix missing mode warnings in nvptx.md, omp part | expand |
On Mon, 17 Jun 2019, Tom de Vries wrote: > Hi Alexander, > > any comments? A couple suggestions (see below), but no serious concerns from my side. > --- a/gcc/config/nvptx/nvptx.c > +++ b/gcc/config/nvptx/nvptx.c > @@ -112,6 +112,17 @@ enum nvptx_data_area > DATA_AREA_MAX > }; > > +rtx > +gen_set_softstack_insn (rtx op) > +{ Here it might be appropriate to have something like gcc_assert (GET_MODE (op) == Pmode); because failure to supply a Pmode operand indicates a bug in the caller and it may be desirable to catch it here; doesn't make sense to allow restoring stack pointer from a 32-bit register with a 64-bit address space. Likewise for other instances of this 'if (... DImode) ... else if (... SImode) ...' pattern in the patch. > + if (GET_MODE (op) == DImode) > + return gen_set_softstack_insn_di (op); > + else if (GET_MODE (op) == SImode) > + return gen_set_softstack_insn_si (op); > + else > + gcc_unreachable (); > +} > + > /* We record the data area in the target symbol flags. */ > #define SYMBOL_DATA_AREA(SYM) \ > (nvptx_data_area)((SYMBOL_REF_FLAGS (SYM) >> SYMBOL_FLAG_MACH_DEP_SHIFT) \ > diff --git a/gcc/config/nvptx/nvptx.md b/gcc/config/nvptx/nvptx.md > index 3ed5296db96..1a676b942b2 100644 > --- a/gcc/config/nvptx/nvptx.md > +++ b/gcc/config/nvptx/nvptx.md > @@ -1071,8 +1071,8 @@ > DONE; > }) > > -(define_insn "set_softstack_insn" > - [(unspec [(match_operand 0 "nvptx_register_operand" "R")] > +(define_insn "set_softstack_insn_<mode>" > + [(unspec [(match_operand:P 0 "nvptx_register_operand" "R")] A purely cosmetic issue, but I'll mention it for completeness: the '_insn' suffix is unnecessary with the '_<mode>' suffix added. This is how e.g. the i386 backend deals with a similar issue, they have 'stack_protect_test' expand and 'stack_protect_test_<mode>' insns. Thanks! Alexander
On 17-06-19 14:35, Alexander Monakov wrote: > On Mon, 17 Jun 2019, Tom de Vries wrote: > >> Hi Alexander, >> >> any comments? > > A couple suggestions (see below), but no serious concerns from my side. > >> --- a/gcc/config/nvptx/nvptx.c >> +++ b/gcc/config/nvptx/nvptx.c >> @@ -112,6 +112,17 @@ enum nvptx_data_area >> DATA_AREA_MAX >> }; >> >> +rtx >> +gen_set_softstack_insn (rtx op) >> +{ > > Here it might be appropriate to have something like > > gcc_assert (GET_MODE (op) == Pmode); > > because failure to supply a Pmode operand indicates a bug in the caller and > it may be desirable to catch it here; doesn't make sense to allow restoring > stack pointer from a 32-bit register with a 64-bit address space. > > Likewise for other instances of this 'if (... DImode) ... else if (... SImode) ...' > pattern in the patch. > Good point, implemented. >> + if (GET_MODE (op) == DImode) >> + return gen_set_softstack_insn_di (op); >> + else if (GET_MODE (op) == SImode) >> + return gen_set_softstack_insn_si (op); >> + else >> + gcc_unreachable (); >> +} >> + >> /* We record the data area in the target symbol flags. */ >> #define SYMBOL_DATA_AREA(SYM) \ >> (nvptx_data_area)((SYMBOL_REF_FLAGS (SYM) >> SYMBOL_FLAG_MACH_DEP_SHIFT) \ >> diff --git a/gcc/config/nvptx/nvptx.md b/gcc/config/nvptx/nvptx.md >> index 3ed5296db96..1a676b942b2 100644 >> --- a/gcc/config/nvptx/nvptx.md >> +++ b/gcc/config/nvptx/nvptx.md >> @@ -1071,8 +1071,8 @@ >> DONE; >> }) >> >> -(define_insn "set_softstack_insn" >> - [(unspec [(match_operand 0 "nvptx_register_operand" "R")] >> +(define_insn "set_softstack_insn_<mode>" >> + [(unspec [(match_operand:P 0 "nvptx_register_operand" "R")] > > A purely cosmetic issue, but I'll mention it for completeness: the '_insn' > suffix is unnecessary with the '_<mode>' suffix added. This is how e.g. the > i386 backend deals with a similar issue, they have 'stack_protect_test' expand > and 'stack_protect_test_<mode>' insns. Updated accordingly, and committed as attached. Thanks, - Tom
On Mon, Jun 17, 2019 at 04:53:24PM +0200, Tom de Vries wrote:
> Updated accordingly, and committed as attached.
Note, current trunk allows one to define expanders that take mode as the
first argument, so you could
(define_insn "@set_softstack_<mode>"
[(unspec [(match_operand:P 0 "nvptx_register_operand" "R")]
UNSPEC_SET_SOFTSTACK)]
"TARGET_SOFT_STACK"
...
and then just use gen_set_softstack (Pmode, arg).
Jakub
[nvptx] Fix missing mode warnings in nvptx.md, omp part Fix these warnings: ... gcc/config/nvptx/nvptx.md:1074:1: warning: operand 0 missing mode? gcc/config/nvptx/nvptx.md:1240:1: warning: operand 0 missing mode? gcc/config/nvptx/nvptx.md:1240:1: warning: operand 1 missing mode? gcc/config/nvptx/nvptx.md:1240:1: warning: operand 2 missing mode? gcc/config/nvptx/nvptx.md:1268:1: warning: operand 0 missing mode? ... Build and reg-tested on x86_64 with nvptx accelerator. 2019-06-17 Tom de Vries <tdevries@suse.de> * config/nvptx/nvptx-protos.h (gen_set_softstack_insn): Declare. * config/nvptx/nvptx.c (gen_set_softstack_insn): New function. * config/nvptx/nvptx.md (define_insn "set_softstack_insn"): Rename to ... (define_insn "set_softstack_insn_<mode>"): ... this. Use P iterator on match_operand 0. (define_insn "omp_simt_enter_insn"): Rename to ... (define_insn "omp_simt_enter_insn_<mode>"): ... this. Use P iterator on match_operand 0, 1 and 2, as well as the unspec_volatile result. (define_expand "omp_simt_enter): Use gen_omp_simt_enter_insn_di and gen_omp_simt_enter_insn_si. (define_expand "omp_simt_exit"): New. (define_insn "omp_simt_exit"): Rename to ... (define_insn "omp_simt_exit_insn_<mode>"): ... this. Use P iterator on match_operand 0. --- gcc/config/nvptx/nvptx-protos.h | 1 + gcc/config/nvptx/nvptx.c | 11 +++++++++++ gcc/config/nvptx/nvptx.md | 38 +++++++++++++++++++++++++++++--------- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/gcc/config/nvptx/nvptx-protos.h b/gcc/config/nvptx/nvptx-protos.h index be09a15e49c..061897a3921 100644 --- a/gcc/config/nvptx/nvptx-protos.h +++ b/gcc/config/nvptx/nvptx-protos.h @@ -57,5 +57,6 @@ extern const char *nvptx_output_set_softstack (unsigned); extern const char *nvptx_output_simt_enter (rtx, rtx, rtx); extern const char *nvptx_output_simt_exit (rtx); extern const char *nvptx_output_red_partition (rtx, rtx); +extern rtx gen_set_softstack_insn (rtx); #endif #endif diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index c53a1ae9f26..37dd10698dd 100644 --- a/gcc/config/nvptx/nvptx.c +++ b/gcc/config/nvptx/nvptx.c @@ -112,6 +112,17 @@ enum nvptx_data_area DATA_AREA_MAX }; +rtx +gen_set_softstack_insn (rtx op) +{ + if (GET_MODE (op) == DImode) + return gen_set_softstack_insn_di (op); + else if (GET_MODE (op) == SImode) + return gen_set_softstack_insn_si (op); + else + gcc_unreachable (); +} + /* We record the data area in the target symbol flags. */ #define SYMBOL_DATA_AREA(SYM) \ (nvptx_data_area)((SYMBOL_REF_FLAGS (SYM) >> SYMBOL_FLAG_MACH_DEP_SHIFT) \ diff --git a/gcc/config/nvptx/nvptx.md b/gcc/config/nvptx/nvptx.md index 3ed5296db96..1a676b942b2 100644 --- a/gcc/config/nvptx/nvptx.md +++ b/gcc/config/nvptx/nvptx.md @@ -1071,8 +1071,8 @@ DONE; }) -(define_insn "set_softstack_insn" - [(unspec [(match_operand 0 "nvptx_register_operand" "R")] +(define_insn "set_softstack_insn_<mode>" + [(unspec [(match_operand:P 0 "nvptx_register_operand" "R")] UNSPEC_SET_SOFTSTACK)] "TARGET_SOFT_STACK" { @@ -1237,10 +1237,10 @@ ;; Patterns for OpenMP SIMD-via-SIMT lowering -(define_insn "omp_simt_enter_insn" - [(set (match_operand 0 "nvptx_register_operand" "=R") - (unspec_volatile [(match_operand 1 "nvptx_nonmemory_operand" "Ri") - (match_operand 2 "nvptx_nonmemory_operand" "Ri")] +(define_insn "omp_simt_enter_insn_<mode>" + [(set (match_operand:P 0 "nvptx_register_operand" "=R") + (unspec_volatile:P [(match_operand:P 1 "nvptx_nonmemory_operand" "Ri") + (match_operand:P 2 "nvptx_nonmemory_operand" "Ri")] UNSPECV_SIMT_ENTER))] "" { @@ -1261,12 +1261,32 @@ cfun->machine->simt_stack_align = MAX (UINTVAL (operands[2]), cfun->machine->simt_stack_align); cfun->machine->has_simtreg = true; - emit_insn (gen_omp_simt_enter_insn (operands[0], operands[1], operands[2])); + if (GET_MODE (operands[0]) == DImode) + emit_insn (gen_omp_simt_enter_insn_di (operands[0], operands[1], + operands[2])); + else if (GET_MODE (operands[0]) == SImode) + emit_insn (gen_omp_simt_enter_insn_si (operands[0], operands[1], + operands[2])); + else + gcc_unreachable (); + DONE; +}) + +(define_expand "omp_simt_exit" + [(match_operand 0 "nvptx_register_operand" "R")] + "" +{ + if (GET_MODE (operands[0]) == DImode) + emit_insn (gen_omp_simt_exit_insn_di (operands[0])); + else if (GET_MODE (operands[0]) == SImode) + emit_insn (gen_omp_simt_exit_insn_si (operands[0])); + else + gcc_unreachable (); DONE; }) -(define_insn "omp_simt_exit" - [(unspec_volatile [(match_operand 0 "nvptx_register_operand" "R")] +(define_insn "omp_simt_exit_insn_<mode>" + [(unspec_volatile [(match_operand:P 0 "nvptx_register_operand" "R")] UNSPECV_SIMT_EXIT)] "" {