[MC,NVPTX] Add MCAsmPrinter support for unsigned-only data directives.

[tra]

Summary

PTX does not support negative values in .bNN data directives and we must
emit such values in a way that ptxas can parse.

MCAsmInfo can now specify whether the target can emit negative values.

[tra]

Updated existing test which produced data directive w/ negative value.

[tra]

Ping!

[hfinkel]

llvm/lib/MC/MCExpr.cpp

71

Will uint64_t always be correct here? Shouldn't this depend on SizeInBytes (like the hex printing does)?

[tra]

llvm/lib/MC/MCExpr.cpp

71

MCConstantExpr::getValue() returns int64_t so casting it to uint64_t should be safe.

I guess I can find the matching unsigned type using std::make_unsigned. E.g:

using unsigned_t = typename std::make_unsigned<decltype(Value)>::type;
OS <<  static_cast<unsigned_t>(v);

[tra]

Use std::make_unsigned to find the matching unsigned type.

[hfinkel]

llvm/lib/MC/MCExpr.cpp

71

I'm not worried about the cast being unsafe, at the C++ level, I'm worried about it printing a number larger than the relevant directive actually accepts. In your test, the directive is .b64, which presumably takes a 64-bit integer, so everything's fine. What if it were .b8 and the printed argument were 18446744073709551613 (or whatever)?

[tra]

Mask out unwanted bits in the unsigned representation of the Value.

[tra]

llvm/lib/MC/MCExpr.cpp

71

Got it. I've masked out the unwanted bits.

[ABataev]

llvm/lib/MC/MCExpr.cpp

71

Does cuda gdb correctly handle the bitfields with this fix? Can you read/write values to/from bitfield in the debugger?

[tra]

llvm/lib/MC/MCExpr.cpp

71

Sort of. It seems to handle field bits within a byte and knows correct bit field length, but struggles with the field which crosses the byte boundary.

E.g:

    struct s {
      unsigned char a : 3;
      unsigned char b : 6;
    } __attribute__((__packed__)) b;

(cuda-gdb) set var b.a=7   # This sets the field correctly.
(cuda-gdb) p b
$7 = {
  a = 7 '\a',
  b = 0 '\000'
}
(cuda-gdb) x/2bx &b
0x7fffca800100: 0x07    0x00
(cuda-gdb) set var b.a=8
warning: Value does not fit in 3 bits. 

(cuda-gdb) set var b.b=0x3f   # This one loses the top bit.
(cuda-gdb) p b
$8 = {
  a = 0 '\000',
  b = 31 '\037'   # Only 5 bits are set.
}
(cuda-gdb) x/2bx &b
0x7fffca800100: 0xf8    0x00  # Should've been 0xf8 0x01

[ABataev]

llvm/lib/MC/MCExpr.cpp

71

Hm, did you try to emit it as hexadecimal?

[hfinkel]

llvm/lib/MC/MCExpr.cpp

71

Hm, did you try to emit it as hexadecimal?

Good point. Maybe we want the option to just force hex printing instead of this new case? Then we can just reuse the existing logic above for that.

[tra]

llvm/lib/MC/MCExpr.cpp

71

Makes no difference. I've run ptxas on .ptx with the constant represented as decimal or as hex. ptxas accepted both and produced bit-for-bit identical binaries.

[hfinkel]

llvm/lib/MC/MCExpr.cpp

71

In that case, I recommend that we go with Alexey's suggestion. Remove this logic and make the condition above if (PrintInHex || (MAI && !MAI->supportsSignedData())) (or something like that).

[tra]

llvm/lib/MC/MCExpr.cpp

71

I'm concerned that the extra 0x and always-on zero-padding will noticeably increase the size of PTX with the debug info as DWARF output seems to be predominantly .b8. Large builds like tensorflow already struggle with their binary size (we already run into ELF reloc overflows in debug builds unless we limit the number of GPUs we target) and this will contribute to the issue.

[hfinkel]

llvm/lib/MC/MCExpr.cpp

71

I'm concerned that the extra 0x and always-on zero-padding will noticeably increase the size of PTX with the debug info as DWARF output seems to be predominantly .b8. Large builds like tensorflow already struggle with their binary size (we already run into ELF reloc overflows in debug builds unless we limit the number of GPUs we target) and this will contribute to the issue.

Ah, okay. I can understand that.

Can you make a test case where this affects the printing of a .b8 directive?

[ABataev]

llvm/lib/MC/MCExpr.cpp

71

Maybe just print the value as hexadecimal if it is negative?

[ABataev]

llvm/lib/MC/MCExpr.cpp

71

Also, I can try to check if another my patch for bitfields works correctly.

[tra]

llvm/lib/MC/MCExpr.cpp

71

The DWARF so far is the only way I've found to get MC to generate constant values in PTX.
Global variables use appropriate .s8/.u8 directives.
Arrays always use .b8 with unsigned byte values.
Nothing else other than DWARF produces .bXX directives, AFAICT.

MC tests run into the problem that they seem to rely on assembling/disassembling something and we can do neither for PTX.

I'm not particularly familiar with MC, so I may be missing something. Ideas/suggestions are welcome.

[ABataev]

llvm/lib/MC/MCExpr.cpp

71

Yes, DWARF is the only place that produces .bXX directives.

[tra]

Print negative values as hex.

[tra]

llvm/lib/MC/MCExpr.cpp

71

Maybe just print the value as hexadecimal if it is negative?

Good idea. Done.

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   /// current section.  If a data directive is set to null, smaller data
   /// directives will be used to emit the large sizes.  Defaults to "\t.byte\t",
   /// "\t.short\t", "\t.long\t", "\t.quad\t"
   const char *Data8bitsDirective;
   const char *Data16bitsDirective;
   const char *Data32bitsDirective;
   const char *Data64bitsDirective;
 
+  /// True if data directives support signed values
+  bool SupportsSignedData = true;
+
   /// If non-null, a directive that is used to emit a word which should be
   /// relocated as a 64-bit GP-relative offset, e.g. .gpdword on Mips.  Defaults
   /// to nullptr.
   const char *GPRel64Directive = nullptr;
 
   /// If non-null, a directive that is used to emit a word which should be
   /// relocated as a 32-bit GP-relative offset, e.g. .gpword on Mips or .gprel32
   /// on Alpha.  Defaults to nullptr.
public:
   bool hasSubsectionsViaSymbols() const { return HasSubsectionsViaSymbols; }
 
   // Data directive accessors.
 
   const char *getData8bitsDirective() const { return Data8bitsDirective; }
   const char *getData16bitsDirective() const { return Data16bitsDirective; }
   const char *getData32bitsDirective() const { return Data32bitsDirective; }
   const char *getData64bitsDirective() const { return Data64bitsDirective; }
+  bool supportsSignedData() const { return SupportsSignedData; }
   const char *getGPRel64Directive() const { return GPRel64Directive; }
   const char *getGPRel32Directive() const { return GPRel32Directive; }
   const char *getDTPRel64Directive() const { return DTPRel64Directive; }
   const char *getDTPRel32Directive() const { return DTPRel32Directive; }
   const char *getTPRel64Directive() const { return TPRel64Directive; }
   const char *getTPRel32Directive() const { return TPRel32Directive; }
 
   /// Targets can implement this method to specify a section to switch to if the

llvm/lib/MC/MCExpr.cpp

 void MCExpr::print(raw_ostream &OS, const MCAsmInfo *MAI, bool InParens) const {
   switch (getKind()) {
   case MCExpr::Target:
     return cast<MCTargetExpr>(this)->printImpl(OS, MAI);
   case MCExpr::Constant: {
     auto Value = cast<MCConstantExpr>(*this).getValue();
     auto PrintInHex = cast<MCConstantExpr>(*this).useHexFormat();
     auto SizeInBytes = cast<MCConstantExpr>(*this).getSizeInBytes();
+    if (Value < 0 && MAI && !MAI->supportsSignedData())
+      PrintInHex = true;
     if (PrintInHex)
       switch (SizeInBytes) {
       default:
         OS << "0x" << Twine::utohexstr(Value);
         break;
       case 1:
         OS << format("0x%02" PRIx64, Value);
         break;
       case 2:
         OS << format("0x%04" PRIx64, Value);
         break;
       case 4:
         OS << format("0x%08" PRIx64, Value);
         break;
       case 8:
         OS << format("0x%016" PRIx64, Value);
         break;
       }
     else
       OS << Value;
     return;
   }
   case MCExpr::SymbolRef: {
     const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(*this);
     const MCSymbol &Sym = SRE.getSymbol();
     // Parenthesize names that start with $ so that they don't look like
     // absolute names.
     bool UseParens =

llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXMCAsmInfo.cpp

NVPTXMCAsmInfo::NVPTXMCAsmInfo(const Triple &TheTriple,
   Data16bitsDirective = nullptr; // not supported
   Data32bitsDirective = ".b32 ";
   Data64bitsDirective = ".b64 ";
   ZeroDirective = ".b8";
   AsciiDirective = nullptr; // not supported
   AscizDirective = nullptr; // not supported
   SupportsQuotedNames = false;
   SupportsExtendedDwarfLocDirective = false;
+  SupportsSignedData = false;
 
   // @TODO: Can we just disable this?
   WeakDirective = "\t// .weak\t";
   GlobalDirective = "\t// .globl\t";
 
   UseIntegratedAssembler = false;
 }

llvm/test/DebugInfo/NVPTX/packed_bitfields.ll

 ; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
 ;   char : 3;
 ;   char a : 6;
 ; } __attribute__((__packed__)) b;
 
 ; Note that DWARF 2 counts bit offsets backwards from the high end of
 ; the storage unit to the high end of the bit field.
 
 ; CHECK: .section .debug_info
 ; CHECK:      .b8 3    // Abbrev {{.*}} DW_TAG_structure_type
 ; CHECK:      .b8 3    // DW_AT_decl_line
 ; CHECK-NEXT: .b8 1    // DW_AT_byte_size
 ; CHECK-NEXT: .b8 6    // DW_AT_bit_size
-; CHECK-NEXT: .b64 -1  // DW_AT_bit_offset
+; Negative offset must be encoded as an unsigned integer.
+; CHECK-NEXT: .b64 0xffffffffffffffff // DW_AT_bit_offset
 ; CHECK-NEXT: .b8 2    // DW_AT_data_member_location
 
 %struct.anon = type { i16 }
 
 @b = global %struct.anon zeroinitializer, align 1, !dbg !0
 
 !llvm.dbg.cu = !{!2}
 !llvm.module.flags = !{!10, !11, !12, !13}