[PATCH] D106380: [SystemZ][z/OS] Initial code to generate assembly files on z/OS

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson added a comment.

Looks good in general, just a few suggestions. The commit message/description should say specifically what functionality this adds.



================
Comment at: llvm/include/llvm/MC/MCContext.h:281
+      }
+    };
+
----------------
I don't think this type is necessary, you could use StringRef directly in the map.


================
Comment at: llvm/include/llvm/MC/MCSectionGOFF.h:32
+public:
+  virtual ~MCSectionGOFF() {}
+
----------------
Is this necessary?


================
Comment at: llvm/include/llvm/MC/MCSectionGOFF.h:37
+                            const MCExpr *Subsection) const override {
+    OS << "\t.section\t\"" << getName() << "\"\n";
+  }
----------------
It might be a good idea to generalise `printName` from `MCSectionELF.cpp` to handle outputting names in quotes with proper escaping.


================
Comment at: llvm/lib/MC/MCContext.cpp:619-631
+  auto IterBool = GOFFUniquingMap.insert(
+      std::make_pair(GOFFSectionKey{Section.str()}, nullptr));
+  auto &Entry = *IterBool.first;
+  if (!IterBool.second)
+    return Entry.second;
+
+  // Otherwise, return a new section.
----------------
Something like this should do the same, assuming the key is changed to `StringRef`


================
Comment at: llvm/test/CodeGen/SystemZ/zos-simple-test.ll:3
+; for the z/OS target
+; RUN: llc < %s -mtriple=s390x-ibm-zos | FileCheck %s
+
----------------
The test doesn't seem directly related to the GOFF section code added here. e.g. it doesn't check any section output, but does check an instruction not added in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106380