[libc-commits] [PATCH] D152486: [libc] Begin implementing a 'libmgpu.a' for math on the GPU

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Code and structuring LGTM but a lot of comments/suggestions around the messaging. For the next step, I really want to see the mass of libc's own implementations increase with more builtin wrappers before increasing the mass of vendor wrappers.



================
Comment at: libc/config/gpu/entrypoints.txt:87
+    libc.src.math.sin
+    libc.src.math.round
+)
----------------
Normally, we add the entire family of functions when adding primitive operations. In this case, you should `roundf` and `roundl` also.


================
Comment at: libc/src/math/gpu/CMakeLists.txt:7
+# basis.
+option(LIBC_GPU_VENDOR_MATH "Use the vendor implementations for math" ON)
+function(add_math_entrypoint_gpu_object name)
----------------
```
Use vendor wrappers for GPU math
```


================
Comment at: libc/src/math/gpu/round.cpp:14
+
+LLVM_LIBC_FUNCTION(double, round, (double x)) { return __builtin_round(x); }
+
----------------
Just saying: Normally, on IEEE-754 conformant HW, the instructions generated by builtins exhibit standard conformance. I am not sure if the GPU floating point instructions are IEEE-754 comformant. If not, we should may be say that on the math status pages somewhere. You can do this separately of course.


================
Comment at: libc/src/math/gpu/vendor/CMakeLists.txt:1
+# To provide math for the GPU we will rely on the vendor libraries. If we find
+# them ahead of time we will import them statically. Otherwise, we will keep
----------------
The wording here has to be fixed. The first sentence should say something like:

```
Math functions not yet available in the libc project, or those not yet tuned for GPU workloads are provided as wrappers over vendor libraries.
```

The current wording will become a license to start adding wrappers for everything. For example: https://reviews.llvm.org/D152575

Also, a lot of this information should be moved to `math/gpu/CMakeLists.txt`.


================
Comment at: libc/src/math/gpu/vendor/CMakeLists.txt:4-5
+# them as external references and expect them to be resolved by the user when
+# they compile. In the future we should try to use a generic implementation for
+# the GPU rather than rely on external libraries.
+
----------------
```
In the future, we will use implementations from the libc project and not provide these wrappers.
```

Use the word "not" even if we do carry an option to wrap vendor libraries for a long time.


================
Comment at: libc/src/math/gpu/vendor/CMakeLists.txt:9
+if(AMDDeviceLibs_FOUND)
+  message(STATUS "Building math for AMDGPU using the ROCm device library.")
+  get_target_property(ocml_path ocml IMPORTED_LOCATION)
----------------
```
AMDGPU math functions falling back to vendor libraries will wrap ROCm device library.
```

Feel free to use more appropriate wording, but the point I want to make is that the message should not appear like a blanket statement.


================
Comment at: libc/src/math/gpu/vendor/CMakeLists.txt:19
+  if (EXISTS ${libdevice_path})
+    message(STATUS "Building math for NVPTX using the CUDA device library.")
+    list(APPEND bitcode_link_flags
----------------
Ditto 

```
NVPTX math functions falling back to vendor libraries will wrap CUDA device library.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152486