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

[Anirudh Prasad via Phabricator via llvm-commits]

anirudhp added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:281
+      }
+    };
+
----------------
tmatheson wrote:
> I don't think this type is necessary, you could use StringRef directly in the map.
Hmm you are correct, since there is only field in the key. But taking into account future maintainability, to me, this seems better to leave in as is.


================
Comment at: llvm/include/llvm/MC/MCSectionGOFF.h:32
+public:
+  virtual ~MCSectionGOFF() {}
+
----------------
tmatheson wrote:
> Is this necessary?
Nope. You're right. There doesn't seem to be a reason for having a virtual destructor here. The default destructor is enough.


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


================
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
+
----------------
tmatheson wrote:
> 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.
I have added a uninitialized global variable to grep for the .bss section and added a new check string(s) to check for both of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106380