[PATCH] D123548: AMDGPU: Emit metadata for the hidden_multigrid_sync_arg conditionally

[Changpeng Fang via Phabricator via llvm-commits]

cfang added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:429
+  if (HiddenArgNumBytes >= 56) {
+    if (!Func.hasFnAttribute("amdgpu-no-multigrid-sync-arg"))
+      emitKernelArg(DL, Int8PtrTy, Align(8), ValueKind::HiddenMultiGridSyncArg);
----------------
foad wrote:
> Personally I would prefer to swap the "if" and "else" parts, since "if (!cond) else ..." reads like a double negative. (Actually a triple negative since it's testing a negative attribute!)
I agree with you in the general sense. But this case seems a little special. The logic is  
    if (function has multigrid... attribute) 
       emit metadata for multigrid.
However, the name of the attribute is  amdgpu-no-multigrid-sync-arg, and thus we use a negate to exactly represent the logic.

In addition, the "else" part essentially does nothing (or things just like cleaning up, or adjusting offset in v5), exchanging "if" and "else" parts looks a little weird because we are more interested in the "if" part.

Since we are doing in this style in this file all the time, I am not going to change in this patch. But this is a good point of suggestion. Thanks. 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:856
                   Args);
+    } else
+      emitKernelArg(DL, Int8PtrTy, Align(8), "hidden_none", Offset, Args);
----------------
foad wrote:
> I think llvm style is to put braces around the "else" part if there are braces around the "if" part.
Really? I have to check llvm style ( I was thinking clang-format will complain if something is wrong).   


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

https://reviews.llvm.org/D123548