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

[daltenty]

Summary

The AIX assembler .space directive can't take a second non-zero argument to fill
with. But LLVM emitFill currently assumes it can. We add a flag to the AsmInfo
to check if non-zero fill is supported, and if we can't zerofill non-zero values
we just splat the .byte directives.

[daltenty]

• Remove include addition

[Xiangling_L]

llvm/include/llvm/MC/MCAsmInfo.h

551

Just a minor comment, is it better to name this function ifZeroDirectiveSupportsNonZeroValue() or doesZeroDirectiveSupportsNonZeroValue() since the return value is a boolean?

llvm/lib/MC/MCAsmStreamer.cpp

1114

Is .byte a AIX only directive? If yes, maybe we should assert OS as AIX as well?
And also is this assertion also apply to above ZeroDirective?

llvm/test/CodeGen/PowerPC/aix-nonzero-zerofill.ll

7

Since the intention of this patch includes and if we can't zerofill non-zero values we just splat the .byte directives., we may also want to check correct format for this testcase ?

[daltenty]

llvm/lib/MC/MCAsmStreamer.cpp

1114

I would expect the .byte directive should be generally applicable (it's supported by GAS among others). The assertion is not, we could in theory emit a non-constant expression for the ZeroDirective and let the assembler do the calculation, but we cannot do this if we are just going to write out the bytes.

[daltenty]

• Update test to add byte splat
• Rename MAI query to doesZeroDirectiveSupportNonZeroValue()

[daltenty]

• Remove erroneously emiting fill length

[jasonliu]

llvm/include/llvm/MC/MCAsmInfo.h

182

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".

llvm/lib/MC/MCAsmStreamer.cpp

1117

Make it a const variable so that we don't need to make a function call every time in the loop?

[daltenty]

llvm/include/llvm/MC/MCAsmInfo.h

182

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.

[daltenty]

• Use IntNumBytes to get the length of fill

[jasonliu]

llvm/include/llvm/MC/MCAsmInfo.h

182

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.

llvm/lib/MC/MCAsmStreamer.cpp

1101–1102

nit: const?

1116

nit:
++i instead of i++
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

1117

instead of ".byte" , use MAI->getData8bitsDirective()?

[daltenty]

• Update comment
• Change assertion into report fatal error
• Address comments

[jasonliu]

LGTM.

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   bool UseDataRegionDirectives = false;
 
   /// True if .align is to be used for alignment. Only power-of-two
   /// alignment is supported.
   bool UseDotAlignForAlignment = false;
 
   //===--- Data Emission Directives -------------------------------------===//
 
-  /// This should be set to the directive used to get some number of zero bytes
-  /// emitted to the current section.  Common cases are "\t.zero\t" and
-  /// "\t.space\t".  If this is set to null, the Data*bitsDirective's will be
-  /// used to emit zero bytes.  Defaults to "\t.zero\t"
+  /// This should be set to the directive used to get some number of zero (and
+  /// non-zero if supported by the directive) bytes emitted to the current
+  /// section. Common cases are "\t.zero\t" and "\t.space\t". Defaults to
+  /// "\t.zero\t"
   const char *ZeroDirective;
 
+  /// This should be set to true if the zero directive supports a value to emit
+  /// other than zero. If this is set to false, the Data*bitsDirective's will be
+  /// used to emit these bytes. Defaults to true.
+  bool ZeroDirectiveSupportsNonZeroValue = true;
+
   /// This directive allows emission of an ascii string with the standard C
   /// escape characters embedded into it.  If a target doesn't support this, it
   /// can be set to null. Defaults to "\t.ascii\t"
   const char *AsciiDirective;
 
   /// If not null, this allows for special handling of zero terminated strings
   /// on this target.  This is commonly supported as ".asciz".  If a target
   /// doesn't support this, it can be set to null.  Defaults to "\t.asciz\t"
bool doesSupportDataRegionDirectives() const {
     return UseDataRegionDirectives;
   }
 
   bool useDotAlignForAlignment() const {
     return UseDotAlignForAlignment;
   }
 
   const char *getZeroDirective() const { return ZeroDirective; }
+  bool doesZeroDirectiveSupportNonZeroValue() const {
+    return ZeroDirectiveSupportsNonZeroValue;
+  }
   const char *getAsciiDirective() const { return AsciiDirective; }
   const char *getAscizDirective() const { return AscizDirective; }
   bool getAlignmentIsInBytes() const { return AlignmentIsInBytes; }
   unsigned getTextAlignFillValue() const { return TextAlignFillValue; }
   const char *getGlobalDirective() const { return GlobalDirective; }
 
   bool doesSetDirectiveSuppressReloc() const {
     return SetDirectiveSuppressesReloc;

llvm/lib/MC/MCAsmStreamer.cpp

void MCAsmStreamer::EmitGPRel32Value(const MCExpr *Value) {
   OS << MAI->getGPRel32Directive();
   Value->print(OS, MAI);
   EmitEOL();
 }
 
 void MCAsmStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
                              SMLoc Loc) {
   int64_t IntNumBytes;
-  if (NumBytes.evaluateAsAbsolute(IntNumBytes) && IntNumBytes == 0)
+  const bool IsAbsolute = NumBytes.evaluateAsAbsolute(IntNumBytes);
+  if (IsAbsolute && IntNumBytes == 0)
     return;
 
   if (const char *ZeroDirective = MAI->getZeroDirective()) {
+    if (MAI->doesZeroDirectiveSupportNonZeroValue() || FillValue == 0) {
       // FIXME: Emit location directives
       OS << ZeroDirective;
       NumBytes.print(OS, MAI);
       if (FillValue != 0)
         OS << ',' << (int)FillValue;
       EmitEOL();
+    } else {
+      if (!IsAbsolute)
+        report_fatal_error(
+            "Cannot emit non-absolute expression lengths of fill.");
+      for (int i = 0; i < IntNumBytes; ++i) {
+        OS << MAI->getData8bitsDirective() << (int)FillValue;
+        EmitEOL();
+      }
+    }
     return;
   }
 
   MCStreamer::emitFill(NumBytes, FillValue);
 }
 
 void MCAsmStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
                              int64_t Expr, SMLoc Loc) {

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.cpp

 }
 
 void PPCXCOFFMCAsmInfo::anchor() {}
 
 PPCXCOFFMCAsmInfo::PPCXCOFFMCAsmInfo(bool Is64Bit, const Triple &T) {
   assert(!IsLittleEndian && "Little-endian XCOFF not supported.");
   CodePointerSize = CalleeSaveStackSlotSize = Is64Bit ? 8 : 4;
   ZeroDirective = "\t.space\t";
+  ZeroDirectiveSupportsNonZeroValue = false;
   SymbolsHaveSMC = true;
 }

llvm/test/CodeGen/PowerPC/aix-nonzero-zerofill.ll

+; RUN: llc -verify-machineinstrs -O0 -mcpu=pwr4 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mcpu=pwr4 -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck %s
+
+@a = constant [4 x i8] c"\02\02\02\02", align 1
+
+; CHECK-NOT:  .space  4,2
+; CHECK:      .byte   2
+; CHECK-NEXT: .byte   2
+; CHECK-NEXT: .byte   2
+; CHECK-NEXT: .byte   2