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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Typos in description: "and also and also", "WE".



================
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);
----------------
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!)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:856
                   Args);
+    } else
+      emitKernelArg(DL, Int8PtrTy, Align(8), "hidden_none", Offset, Args);
----------------
I think llvm style is to put braces around the "else" part if there are braces around the "if" part.


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

https://reviews.llvm.org/D123548