[libc-commits] [PATCH] D151282: [libc] Add initial support for 'puts' and 'fputs' to the GPU

[Joseph Huber via Phabricator via libc-commits]

jhuber6 marked an inline comment as done.
jhuber6 added inline comments.


================
Comment at: libc/src/__support/File/file.h:43
+// out the buffering portions of the code in the general implementation.
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
+  static constexpr bool ENABLE_BUFFER = false;
----------------
sivachandra wrote:
> jhuber6 wrote:
> > sivachandra wrote:
> > > Sorry, forgot to mention this in the earlier post: Just pointing out that files opened even with `fopencookie` will become unbuffered.
> > > 
> > > That we have new switch means we should add tests for it on non-GPU platforms also. So, one way would be to do something like this:
> > > 1. Add a compile option, say `LIBC_COPT_UNBUFFERED_FILE` and update this conditional to:
> > > ```
> > > #if defined(LIBC_TARGET_ARCH_IS_GPU) || defined(LIBC_COPT_UNBUFFERED_FILE)
> > > ```
> > > 2. The above flag will allow to setup up a separate platform independent test only target for unbuffered files.
> > > 3. Add a test for unbuffered files which depends on the ubuffered file target.
> > It would be fine to have other users of unbuffered IO, maybe for a more minimal implementation.
> I agree that it is fine to have this switch. My comment about `fopencookie` was just FYI. But, to protect accidental regressions around this switch, I am suggesting that you should add a test for it even for non-GPU platforms. 
I'm not sure how to add a test on a non-GPU platform since it's a build-time thing.


================
Comment at: libc/src/__support/File/gpu/dir.cpp:16
+ErrorOr<int> platform_opendir(const char *name) {
+  return __llvm_libc::Error(-1);
+}
----------------
sivachandra wrote:
> jhuber6 wrote:
> > sivachandra wrote:
> > > If they are just erroring out, why are they required?
> > Wasn't sure if this was required for linking somewhere.
> You are seeing some linker error if you don't add this file?
The CMake expects it currently so I just provide an empty file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151282/new/

https://reviews.llvm.org/D151282