On 9/5/18 7:42 AM, Andrew Stubbs wrote: > This part initially failed to send due to size. > > Here's part 2. > > 0021-gcn-port-pt2.patch [ ... ] You've already addressed Joseph's comments in a follow-up.
> > diff --git a/gcc/config/gcn/gcn.c b/gcc/config/gcn/gcn.c > new file mode 100644 > index 0000000..7e59b06 > --- /dev/null > +++ b/gcc/config/gcn/gcn.c > @@ -0,0 +1,6161 @@ > +/* Copyright (C) 2016-2018 Free Software Foundation, Inc. > + > + This file is free software; you can redistribute it and/or modify it under > + the terms of the GNU General Public License as published by the Free > + Software Foundation; either version 3 of the License, or (at your option) > + any later version. > + > + This file is distributed in the hope that it will be useful, but WITHOUT > + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + for more details. > + > + You should have received a copy of the GNU General Public License > + along with GCC; see the file COPYING3. If not see > + <http://www.gnu.org/licenses/>. */ > + > +/* {{{ Includes. */ > + > +/* We want GET_MODE_SIZE et al to return integers, please. */ > + > +static tree > +gcn_handle_amdgpu_hsa_kernel_attribute (tree *node, tree name, > + tree args, int, bool *no_add_attrs) > +{ > + if (TREE_CODE (*node) != FUNCTION_TYPE > + && TREE_CODE (*node) != METHOD_TYPE > + && TREE_CODE (*node) != METHOD_TYPE > + && TREE_CODE (*node) != FIELD_DECL > + && TREE_CODE (*node) != TYPE_DECL) METHOD_TYPE tested twice here. Might as well use FUNC_OR_METHOD_TYPE_P. >> + > +/* Return true is REG is a valid place to store a pointer, > + for instructions that require an SGPR. > + FIXME rename. */ > + > +static bool > +gcn_address_register_p (rtx reg, machine_mode mode, bool strict) > +{ > + if (GET_CODE (reg) == SUBREG) > + reg = SUBREG_REG (reg); > + > + if (!REG_P (reg)) > + return false; > + > + if (GET_MODE (reg) != mode) > + return false; > + > + int regno = REGNO (reg); > + > + if (regno >= FIRST_PSEUDO_REGISTER) > + { > + if (!strict) > + return true; > + > + if (!reg_renumber) > + return false; > + > + regno = reg_renumber[regno]; > + } > + > + return (regno < 102 || regno == M0_REG > + || regno == ARG_POINTER_REGNUM || regno == FRAME_POINTER_REGNUM); Consider using a symbolic name for "102" :-) > + > +/* Generate epilogue. Called from gen_epilogue during pro_and_epilogue pass. > + > + See gcn_expand_prologue for stack details. */ > + > +void > +gcn_expand_epilogue (void) You probably need a barrier in here to ensure that the scheduler doesn't move an aliased memory reference into the local stack beyond the stack adjustment. You're less likely to run into it because you eliminate frame pointers fairly aggressively, but it's still the right thing to do. > + > +/* Implement TARGET_LEGITIMATE_COMBINED_INSN. > + > + Return false if the instruction is not appropriate as a combination of two > + or more instructions. */ > + > +bool > +gcn_legitimate_combined_insn (rtx_insn *insn) > +{ > + rtx pat = PATTERN (insn); > + > + /* The combine pass tends to strip (use (exec)) patterns from insns. This > + means it basically switches everything to use the *_scalar form of the > + instructions, which is not helpful. So, this function disallows such > + combinations. Unfortunately, this also disallows combinations of > genuine > + scalar-only patterns, but those only come from explicit expand code. > + > + Possible solutions: > + - Invent TARGET_LEGITIMIZE_COMBINED_INSN. > + - Remove all (use (EXEC)) and rely on md_reorg with "exec" attribute. > + */ This seems a bit hokey. Why specifically is combine removing the USE? > + > +/* If INSN sets the EXEC register to a constant value, return the value, > + otherwise return zero. */ > + > +static > +int64_t gcn_insn_exec_value (rtx_insn *insn) Nit. Make sure the function's name is in column 0 by moving the return type to the previous line. > +{ > + if (!NONDEBUG_INSN_P (insn)) > + return 0; > + > + rtx pattern = PATTERN (insn); > + > + if (GET_CODE (pattern) == SET) > + { > + rtx dest = XEXP (pattern, 0); > + rtx src = XEXP (pattern, 1); > + > + if (GET_MODE (dest) == DImode > + && REG_P (dest) && REGNO (dest) == EXEC_REG > + && CONST_INT_P (src)) > + return INTVAL (src); > + } > + > + return 0; > +} > + > +/* Sets the EXEC register before INSN to the value that it had after > + LAST_EXEC_DEF. The constant value of the EXEC register is returned if > + known, otherwise it returns zero. */ > + > +static > +int64_t gcn_restore_exec (rtx_insn *insn, rtx_insn *last_exec_def, > + int64_t curr_exec, bool curr_exec_known, > + bool &last_exec_def_saved) Similarly. Probably worth a check through all the code to catch any other similar nits. > + > + CLEAR_REG_SET (&live); > + > + /* "Manually Inserted Wait States (NOPs)." > + > + GCN hardware detects most kinds of register dependencies, but there > + are some exceptions documented in the ISA manual. This pass > + detects the missed cases, and inserts the documented number of NOPs > + required for correct execution. */ How unpleasant :( But if it's what you need to do, so be it. I'll assume the compiler is the right place to do this -- though some ports handle this kind of stuff in the assembler or linker. > diff --git a/gcc/config/gcn/gcn.h b/gcc/config/gcn/gcn.h > new file mode 100644 > index 0000000..74f0773 > --- /dev/null > +++ b/gcc/config/gcn/gcn.h [ ... ] > +/* Disable the "current_vector_size" feature intended for > + AVX<->SSE switching. */ Guessing you just copied the comment, you probably want to update it to not refer to AVX/SSE. You probably need to define the safe-speculation stuff (TARGET_SPECULATION_SAFE_VALUE). > + > +; "addptr" is the same as "add" except that it must not write to VCC or SCC > +; as a side-effect. Unfortunately GCN3 does not have a suitable instruction > +; for this, so we use a split to save and restore the condition code. > +; This pattern must use "Sg" instead of "SD" to prevent the compiler > +; assigning VCC as the destination. > +; FIXME: Provide GCN5 implementation I worry about the save/restore aspects of this. Haven't we discussed this somewhere?!? Generally I don't see major concerns. THere's some minor things to fix. As far as the correctness of the code you're generating, well, I'm going have to assume you've got that right and will own addressing bugs in that space. My inclination would be to have this go forward into gcc-9 as the minor issues are wrapped up. jeff
