[PATCH] D73554: [AIX] Don't use a zero fill with a second parameter

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:182
+  /// other than zero. Defaults to true.
+  bool ZeroDirectiveSupportsNonZeroValue = true;
+
----------------
daltenty wrote:
> jasonliu wrote:
> > Have we consider rename the ZeroDirective to FillDirective and fix the comments there?
> > Since this ZeroDirective did not convey what it really means.
> > And this query is more like "FillDirectiveSupportsNonZeroFill".
> That's not really quite right either. Fill directive seems to imply `.fill` which has different semantics again (and doesn't exist for the AIX assembler). And if a fill directive only supports zero's isn't it really a zero directive? So maybe there should actually be both? 
> 
> In any case I'm not sure how much we really want to dig into the naming issue for this patch. We'd likely have to make a fairly widespread change.
If we don't want to be too pedantic about naming, let's at least fix the comments about ZeroDirective. From the comment, it says it's used to emit zero bytes. That's definitely not the case here. 
These two lines needs to be fixed: 
This should be set to the directive used to get some number of **zero bytes** emitted to the current section. 
If this is set to null, the Data*bitsDirective's will be used to emit **zero bytes**.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1101
   int64_t IntNumBytes;
-  if (NumBytes.evaluateAsAbsolute(IntNumBytes) && IntNumBytes == 0)
+  bool IsAbsolute = NumBytes.evaluateAsAbsolute(IntNumBytes);
+  if (IsAbsolute && IntNumBytes == 0)
----------------
nit: const?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1116
+             "Cannot emit non-absolute expression lengths of fill.");
+      for (int i = 0; i < IntNumBytes; i++) {
+        OS << "\t.byte\t";
----------------
nit: 
++i instead of i++
https://llvm.org/docs/CodingStandards.html#prefer-preincrement


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1117
+      for (int i = 0; i < IntNumBytes; i++) {
+        OS << "\t.byte\t";
+        OS << (int)FillValue;
----------------
instead of ".byte" , use MAI->getData8bitsDirective()? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73554