[PATCH] D61493: AMDGPU/MC: Add .amdgpu_lds directive

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: include/llvm/BinaryFormat/ELF.h:1429
+enum {
+  SHN_AMDGPU_LDS = 0xff00, // Variable in LDS; symbol encoded like SHN_COMMON
+};
----------------
arsenm wrote:
> Is there a particular reason for this value?
Yes, this is the first processor-specific section index. SHN_HEXAGON_SCOMMON* are defined similarly.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:601-602
+  if (Machine == EM_AMDGPU) {
+    return Index == SHN_AMDGPU_LDS;
+  } else if (Machine == EM_HEXAGON) {
     switch (Index) {
----------------
arsenm wrote:
> No else after return
Done.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:630
   case SYMBOL_COMMON:
-  case SYMBOL_HEXAGON_SCOMMON:
+  case SYMBOL_HEXAGON_SCOMMON: // aliases SYMBOL_AMDGPU_LDS
   case SYMBOL_HEXAGON_SCOMMON_2:
----------------
arsenm wrote:
> This seems like a problem?
After some more meditation on this, I think the correct approach is to treat all OS- or processor-specific reserved sections as acceptable.

Otherwise, the code would have to be refactored with knowledge about what the current OS and processor are at this point, which seems a major and unnecessary change.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61493