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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/config/gpu/entrypoints.txt:88
-    libc.src.stdio.fclose
-    libc.src.stdio.fopen
     libc.src.stdio.stdin
----------------
I suppose you want to add these back in a future patch?


================
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)
----------------
In a different patch, you can may be get rid of the GPU mutex also?


================
Comment at: libc/src/stdio/CMakeLists.txt:33
     libc.src.__support.File.file
     libc.src.__support.File.platform_file
 )
----------------
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)
...
```


================
Comment at: libc/src/stdio/CMakeLists.txt:397
-    libc.src.__support.File.file
-    libc.src.__support.File.platform_file
 )
----------------
These should be listed - the actual global vars come from the deps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157427