On 09/11/2018 18:48, Jeff Law wrote:
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index 0c5b264..6f68257 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -429,9 +429,11 @@ LIB2ADD += enable-execute-stack.c
# While emutls.c has nothing to do with EH, it is in LIB2ADDEH*
# instead of LIB2ADD because that's the way to be sure on some targets
# (e.g. *-*-darwin*) only one copy of it is linked.
+ifneq ($(enable_emutls),no)
LIB2ADDEH += $(srcdir)/emutls.c
LIB2ADDEHSTATIC += $(srcdir)/emutls.c
LIB2ADDEHSHARED += $(srcdir)/emutls.c
+endif
Why is this needed? Are you just trying to cut out stuff you don't need
in the quest for smaller code or does this cause a more direct problem?
This dates back to when that code wouldn't compile. It also surprised me
that --disable-emutls didn't do it (but this stuff is long ago now, so I
don't recall the details of that).
Anyway, the code compiles now, so I can remove this hunk.
+/* Provide an entry point symbol to silence a linker warning. */
+void _start() {}
This seems wrong. I realize you're trying to quiet a linker warning
here, but for the case where you're creating GCN executables (testing?)
this should probably be handled by the C-runtime or linker script.
We're using an LLVM linker, so I'd rather fix things here than there.
Anyway, I plan to make this a proper kernel and use it to run static
constructors, one day. Possibly it should be in Newlib, but then the
"ctors" code is found in crtstuff and libgcc2, so I don't know?
diff --git a/libgcc/config/gcn/gomp_print.c b/libgcc/config/gcn/gomp_print.c
new file mode 100644
index 0000000..41f50c3
--- /dev/null
+++ b/libgcc/config/gcn/gomp_print.c
[ ... ]
Would this be better in libgomp? Oh, you addressed that in the
prologue. Feels like libgomp would be better to me, but I can
understand the rationale behind wanting it in libgcc.
Now that printf works, possibly it should be moved back. There's no
debugger for this target, so these routines are my usual means for
debugging stuff, and libgomp isn't built in the config used to run the
testsuite.
I won't comment on the static sizes since this apparently has to match
something already in existence.
Yeah, this is basically a shared memory interface. I plan to implement a
proper circular buffer, etc., etc., etc., but there's a lot to do.
+
+void
+gomp_print_string (const char *msg, const char *value)
+{
+ struct printf_data *output = reserve_print_slot ();
+ output->type = 2; /* String. */
+
+ strncpy (output->msg, msg, 127);
+ output->msg[127] = '\0';
+ strncpy (output->text, value, 127);
+ output->text[127] = '\0';
+
+ asm ("" ::: "memory");
+ output->written = 1;
+}
I'm not familiar with the GCN memory model, but your asm is really just
a barrier for the compiler. Do you need any kind of hardware fencing
here? Similarly for other instances.
As long as the compiler doesn't reorder the write instructions then this
is fine, as is. The architecture does not reorder writes in hardware.
That said, actually the updated version I'm preparing has additional L1
cache flushes to make absolutely sure the data are written to the L2
cache memory in order. I did this when investigating a problem to make
sure I wasn't losing debug output, and even though I found that I was
not, I kept the patch anyway.
All these functions probably need a little comment on their purpose and
arguments.
Understood.
Note some of the divmod stuff recently changed. You may need minor
updates as a result of those patches. See:
OK, thanks for the heads up.
And thanks for the review. I'm planning to post a somewhat-updated V2
patch set any week now.
Andrew
