This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [OpenACC 2/11] PTX backend changes
- From: Bernd Schmidt <bschmidt at redhat dot com>
- To: Nathan Sidwell <nathan at acm dot org>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Cc: Jakub Jelinek <jakub at redhat dot com>, Jason Merrill <jason at redhat dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>
- Date: Thu, 22 Oct 2015 16:04:21 +0200
- Subject: Re: [OpenACC 2/11] PTX backend changes
- Authentication-results: sourceware.org; auth=none
- References: <5627DD78 dot 9040302 at acm dot org> <5627E303 dot 5050701 at acm dot org>
On 10/21/2015 09:09 PM, Nathan Sidwell wrote:
At the beginning of a partitioned region, we have to propagate live
register state and stack frame from engine-zero to the other engines
(just as would happen on a regular 'fork' call).
This is something I'm not terribly happy about, but since I have no
alternative to offer, it's fine. I expect it could turn out to be a
glass jaw of the implementation though. Speaking of which - do you have
any recent speedup numbers for nontrivial OpenACC programs?
Some other minor nitpicks:
+ /* Emit fork at all levels, this helps form SESE regions.. */
Could expand the comment, it doesn't help me understand the issue. Also,
punctuation.
+/* Structure used when generating a worker-level spill or fill. */
+
+struct wcast_data_t
+{
+ rtx base;
+ rtx ptr;
+ unsigned offset;
+};
Could document the members.
+/* Loop structure of the function.The entire function is described as
Whitespace.
+ // Clear visited flag, for use by parallel locator */
Odd comment style :-)
+
+static void
+nvptx_propagate (basic_block block, rtx_insn *insn, propagate_mask rw,
+ rtx (*fn) (rtx, propagate_mask,
+ unsigned, void *), void *data)
Break that out into a typedef?
+ /* Allow worker function to initialize anything needed */
Punctuation.
+ default:break;
Formatting.
+ if (par->mask & GOMP_DIM_MASK (GOMP_DIM_MAX))
+ { /* No propagation needed for a call. */ }
+ else if (par->mask & GOMP_DIM_MASK (GOMP_DIM_WORKER))
Ok that looks weird with the open brace on the line before the else. I
think the standard practice is to just use "/* .. */;", but possibly
just invert the if condition and move the else branches into it.
+ unsigned me = par->mask
+ & (GOMP_DIM_MASK (GOMP_DIM_WORKER) | GOMP_DIM_MASK (GOMP_DIM_VECTOR));
Formatting. Maybe have extra defines for the masks so you don't have to
spell GOMP_DIM_MASK everytime?
+ if ((outer | me) & GOMP_DIM_MASK (mode))
+ { /* Mode is partitioned: no neutering. */ }
+ else if (!(modes & GOMP_DIM_MASK (mode)))
+ { /* Mode is not used: nothing to do. */ }
Same issue as above. Whitespace in comment.
+ else
+ { /* Parent will skip this parallel itself. */ }
Here too - actually no need to have an empty else at all.
+ "%.\\tmov.b64\\t{%0,%1}, %2;")
> + "%.\\tmov.b64\\t%0, {%1,%2};")
Might want to add a space after the comma. I can see arguments for and
against, so do what you like.
Bernd