[PATCH] D76357: [AMDGPU] Implement wave32 DWARF register mapping

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:253-255
+    if (WavefrontSize == 32)
+      return AMDGPUDwarfFlavour::Wave32;
+    return AMDGPUDwarfFlavour::Wave64;
----------------
Ternary operator


================
Comment at: llvm/unittests/MC/AMDGPU/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
----------------
Missing the file with check for AMDGPU registered target? I'm not sure exactly how that works with unit tests, but it's done for GlobalISel unit tests with AArch64


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:19
+void InitializeTargets() {
+  static bool g_initialized = false;
+  if (!g_initialized) {
----------------
probably needs call_once


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:30
+
+  const char *Triple = "amdgcn";
+  std::string Error;
----------------
Can this be a StringRef? You also don't reuse it below. This should probably also use the full 3 component triple


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:33
+  const Target *T = TargetRegistry::lookupTarget(Triple, Error);
+  EXPECT_TRUE(T);
+
----------------
This should be an ASSERT_TRUE(t) << Error


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:38
+      "amdgcn", "gfx1010", "+wavefrontsize64", Options, None, None));
+  EXPECT_TRUE(TM);
+
----------------
ASSERT_TRUE


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:44
+  // PC_64 => 16
+  MCRegister llvmPCReg(*MRI->getLLVMRegNum(16, false));
+  EXPECT_EQ(16, MRI->getDwarfRegNum(llvmPCReg, false));
----------------
Don't need the llvm prefixes on all of these


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:90
+  const Target *T = TargetRegistry::lookupTarget(Triple, Error);
+  EXPECT_TRUE(T);
+
----------------
Same as above


================
Comment at: llvm/unittests/MC/AMDGPU/DwarfRegMappings.cpp:95
+      "amdgcn", "gfx1010", "-wavefrontsize64", Options, None, None));
+  EXPECT_TRUE(TM);
+
----------------
Same as above


================
Comment at: llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp:27
+  const Target *T = TargetRegistry::lookupTarget(Triple, Error);
+  EXPECT_TRUE(T);
+
----------------
ASSERT_TRUE(T) << Error


================
Comment at: llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp:91
+  auto TM = static_cast<GCNTargetMachine *>(T->createTargetMachine(
+      "amdgcn", "gfx1010", "-wavefrontsize64", Options, None, None));
+  EXPECT_TRUE(TM);
----------------
Should probably repeat test with more combinations of wavefront size subtarget features


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76357