Since Jakub appears to be busy, I'll give my 2 cents.
On 08/25/2015 03:29 PM, Nathan Sidwell wrote:
I did rename the GOACC_parallel entry point to GOACC_parallel_keyed and
provide a forwarding function. However, as the mkoffload data is
incompatible, this is probably overkill. I've had to increment the
(just committed) version number to detect the change in data
representation. So any attempt to run an old binary with a new libgomp
will fail at the loading point.
Fail how? Jakub has requested that it works but falls back to
unaccelerated execution, can you confirm this is what you expect to
happen with this patch?
+/* Varadic launch arguments. */
+#define GOMP_LAUNCH_END 0 /* End of args, no dev or op */
+#define GOMP_LAUNCH_DIM 1 /* Launch dimensions, op = mask */
+#define GOMP_LAUNCH_ASYNC 2 /* Async, op = cst val if not MAX */
+#define GOMP_LAUNCH_WAIT 3 /* Waits, op = num waits. */
+#define GOMP_LAUNCH_CODE_SHIFT 28
+#define GOMP_LAUNCH_DEVICE_SHIFT 16
+#define GOMP_LAUNCH_OP_SHIFT 0
+#define GOMP_LAUNCH_PACK(CODE,DEVICE,OP) \
+ (((CODE) << GOMP_LAUNCH_CODE_SHIFT) \
+ | ((DEVICE) << GOMP_LAUNCH_DEVICE_SHIFT) \
+ | ((OP) << GOMP_LAUNCH_OP_SHIFT))
+#define GOMP_LAUNCH_CODE(X) (((X) >> GOMP_LAUNCH_CODE_SHIFT) & 0xf)
+#define GOMP_LAUNCH_DEVICE(X) (((X) >> GOMP_LAUNCH_DEVICE_SHIFT) & 0xfff)
+#define GOMP_LAUNCH_OP(X) (((X) >> GOMP_LAUNCH_OP_SHIFT) & 0xffff)
+#define GOMP_LAUNCH_OP_MAX 0xffff
I probably would have used something simpler, like a code/device/op
argument triplet, but I guess this ok.
- if (num_waits)
+ va_start (ap, kinds);
+ /* TODO: This will need amending when device_type is implemented. */
I'd expect that this will check whether the device type in the argument
is either zero (or whatever indicates all devices) or matches the
current device. Is that what you intend?
+ while (GOMP_LAUNCH_PACK (GOMP_LAUNCH_END, 0, 0)
+ != (tag = va_arg (ap, unsigned)))
That's a somewhat non-idiomatic way to write this, with the constant
first and not obviously a constant. I'd initialize a variable with the
constant before the loop.
+ assert (!GOMP_LAUNCH_DEVICE (tag));
Uh, that seems unfriendly, and not exactly forwards compatible. Can that
fail a bit more gracefully? (Alternatively, implement the device_type
stuff now so that we don't have TODOs in the code and don't have to
worry about compatibility issues.)
+ if (num_waits > 8)
+ gomp_fatal ("Too many waits for legacy interface");
How did you arrive at this number?
+GOACC_2.0,1 {
+ global:
+ GOACC_parallel_keyed;
+} GOACC_2.0;
Did you mean to use a comma?
+ if (dims[GOMP_DIM_GANG] != 1)
+ GOMP_PLUGIN_fatal ("non-unity num_gangs (%d) not supported",
+ dims[GOMP_DIM_GANG]);
+ if (dims[GOMP_DIM_WORKER] != 1)
+ GOMP_PLUGIN_fatal ("non-unity num_workers (%d) not supported",
+ dims[GOMP_DIM_WORKER]);
I see that this is just moved here (which is good), but is this still a
limitation? Or is that on trunk only?
+ for (comma = "", id = var_ids; id; comma = ",", id = id->next)
+ fprintf (out, "%s\n\t%s", comma, id->ptx_name);
The comma trick is new to me, I'll have to remember this one.
+static void
+set_oacc_fn_attrib (tree fn, tree clauses, vec<tree> *args)
+tree
+get_oacc_fn_attrib (tree fn)
These need function comments.
+{
+ /* Must match GOMP_DIM ordering. */
+ static const omp_clause_code ids[] =
+ {OMP_CLAUSE_NUM_GANGS, OMP_CLAUSE_NUM_WORKERS, OMP_CLAUSE_VECTOR_LENGTH};
Formatting. No = at the end of a line, and whitespace around braces.
@@ -9150,6 +9245,7 @@ expand_omp_target (struct omp_region *re
}
gimple g;
+ bool tagging = false;
/* The maximum number used by any start_ix, without varargs. */
That looks misindented, but may be an email client thing.
+ else if (!tagging)
Oh... so tagging controls two different methods for constructing
argument lists, one for GOACC_parallel and the other for whatever OMP
uses? That's a bit unfortunate, I'll need to think about it for a bit or
defer to Jakub.
Looks reasonable otherwise.
Bernd
