[Mlir-commits] [llvm] [mlir] [mlir] Replace MLIR_ENABLE_ROCM_CONVERSIONS with LLVM_HAS_AMDGPU_TARGET (PR #182652)

[Aaron Siddhartha Mondal]

================
@@ -41,7 +41,7 @@ expand_template(
         "@LLVM_HAS_NVPTX_TARGET@": "0",
         "@LLVM_INCLUDE_SPIRV_TOOLS_TESTS@": "0",
         "@MLIR_ENABLE_CUDA_RUNNER@": "0",
-        "@MLIR_ENABLE_ROCM_CONVERSIONS@": "0",
+        "@LLVM_HAS_AMDGPU_TARGET@": "0",
----------------
aaronmondal wrote:

The nvptx-related build that you mention looks broken to me here as well because that's hardcoded to 0 as well.

So neither the AMDGPU build nor the NVPTX build are actually correct. I can't speak to whether the change to the defines is desirable on the MLIR side, but regarding the Bazel side this isn't really "fixing" something.

The right way to fix this is to use conditionals like so that mirror what cmake is doing:

```python
"@SOMEOPTION@": "A" if somethingisenabled else "B"
```

E.g. for the option you removed it would look something roughly like this:

```python
"@MLIR_ENABLE_ROCM_CONVERSIONS@": "1" if (targets.contains("amdgpu")) else "0"
```

So if you e.g. enable the amdgpu target you'd want to import that knowledge here. Without such conditionals you'd just unconditionally disable the tests for AMDGPU just how they're currently incorrectly disabled for NVPTX (or at least you'd build tests with incorrect defines, risking false positives in tests).

Since NVPTX is already handled wrongly it doesn't seem like it would be the end of the world to continue the pattern for AMDGPU, but then the PR comment stating that this is fixing something is misleading. It's just pattern matching a wrong pattern to something slightly more consistent.

Here is a more comprehensive example on how these kinds of conditionals "should" look like IMO: https://github.com/llvm/llvm-project/pull/126729/changes

https://github.com/llvm/llvm-project/pull/182652