[Mlir-commits] [mlir] [mlir][spirv] Fix lookup logic `spirv.target_env` for `gpu.module` (PR #147262)

[Jaeho Kim]

================
@@ -385,6 +385,11 @@ LogicalResult GPUModuleConversion::matchAndRewrite(
   if (auto attr = moduleOp->getAttrOfType<spirv::TargetEnvAttr>(
           spirv::getTargetEnvAttrName()))
     spvModule->setAttr(spirv::getTargetEnvAttrName(), attr);
+  if (ArrayAttr targets = moduleOp.getTargetsAttr()) {
+    for (Attribute targetAttr : targets)
+      if (auto spirvTargetEnvAttr = dyn_cast<spirv::TargetEnvAttr>(targetAttr))
+        spvModule->setAttr(spirv::getTargetEnvAttrName(), spirvTargetEnvAttr);
----------------
oojahooo wrote:

After giving it some more thought, I think selecting the first target env makes more sense.

In the `gpu-module-to-binary pass`, the SPIR-V serialization process currently picks the first `spirv.module` for serialization when there are multiple `spirv.module`s inside a `gpu.module` ([relevant code](https://github.com/llvm/llvm-project/blob/9cd2413601f809fdb6c011743eb37b1321d5ab17/mlir/lib/Target/SPIRV/Target.cpp#L73)). So, even if `spirv.modules` are created for all target env in the `targets` array (assuming the order is preserved), only the one generated from the first target env will actually be serialized.
Also, in the same pass, an `OffloadingLLVMTranslationAttrInterface` is constructed to handle offloading for the targets attribute. This enables target selection during the final `mlir-translate` phase. As the current implementation of the default `SelectObjectAttr` selects the first object when no parameters are provided([relevant desc](https://github.com/llvm/llvm-project/blob/236b315a98900468cc749a618e2204c0d499876a/mlir/include/mlir/Dialect/GPU/IR/CompilationAttrs.td#L265)), I thought it would be more consistent to align with that behavior.

If you agree with this reasoning, I’d be happy to update the code accordingly to explicitly select the first target env.

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