[PATCH] D106380: [SystemZ][z/OS] Initial code to generate assembly files on z/OS
Anirudh Prasad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 21 09:22:53 PDT 2021
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
More information about the llvm-commits
mailing list