[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

69

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

[tra]

llvm/lib/MC/MCExpr.cpp

69

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

69

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

69

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

[ABataev]

llvm/lib/MC/MCExpr.cpp

69

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

69

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

69

Hm, did you try to emit it as hexadecimal?

[hfinkel]

llvm/lib/MC/MCExpr.cpp

69

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

69

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

69

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

69

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

69

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

69

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

[ABataev]

llvm/lib/MC/MCExpr.cpp

69

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

[tra]

llvm/lib/MC/MCExpr.cpp

69

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

69

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

[tra]

Print negative values as hex.

[tra]

llvm/lib/MC/MCExpr.cpp

69

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

if (PrintInHex)
         break;
       case 4:
         OS << format("0x%08" PRIx64, Value);
         break;
       case 8:
         OS << format("0x%016" PRIx64, Value);
         break;
       }
+    else if (MAI && !MAI->supportsSignedData())
+      OS << static_cast<uint64_t>(Value);
     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

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 18446744073709551615 // 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}