[libc-commits] [PATCH] D157427: [libc] Rework the file handling for the GPU

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added inline comments.


================
Comment at: libc/config/gpu/entrypoints.txt:88
-    libc.src.stdio.fclose
-    libc.src.stdio.fopen
     libc.src.stdio.stdin
----------------
sivachandra wrote:
> I suppose you want to add these back in a future patch?
Yes, wanted to keep this patch smaller.


================
Comment at: libc/src/__support/File/CMakeLists.txt:1
-if(NOT (TARGET libc.src.__support.threads.mutex))
+if(NOT (TARGET libc.src.__support.threads.mutex)
+   OR LIBC_TARGET_ARCHITECTURE_IS_GPU)
----------------
sivachandra wrote:
> In a different patch, you can may be get rid of the GPU mutex also?
We also use that for `atexit`.


================
Comment at: libc/src/stdio/CMakeLists.txt:33
     libc.src.__support.File.file
     libc.src.__support.File.platform_file
 )
----------------
sivachandra wrote:
> This can be confusing or misleading. For example, if GPU libc does not use the buffered file abstraction, why do we have these dependencies listed here? I think a structuring like this might be more natural and explicit:
> 
> ```
> # All all entrypoints with generic implementations will be listed with a "generic_" prefix in the name.
> # For example:
> add_entrypoint_object(
>   generic_fclose
>   ...
> )
> 
> # The helper function chooses the OS specialization if available, else falls back to the generic version.
> function(add_stdio_entrypoint_object name)
>   if(TARGET libc.src.stdio.${LIBC_TARGET_OS}.${name})
>     add_entrypoint_object(
>       ${name}
>       ALIAS
>       DEPENDS
>         .${LIBC_TARGET_OS}.${name}
>     )
>   elif(TARGET libc.src.stdio.generic_${name})
>     add_entrypoint_object(
>       ${name}
>       ALIAS
>       DEPENDS
>         .generic_${entrypoint}
>     )
> endfunction(add_stdio_entrypoint_object)
> 
> add_stdio_entrypoint_object(fclose)
> ...
> ```
Alright, I can do that and then only change the files that are overridden currently.


================
Comment at: libc/src/stdio/CMakeLists.txt:397
-    libc.src.__support.File.file
-    libc.src.__support.File.platform_file
 )
----------------
sivachandra wrote:
> These should be listed - the actual global vars come from the deps.
So we'll need an `if(LIBC_TARGET_ARCHITECTURE_IS_GPU)` version since the `file` dependency isn't built.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157427