This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tra on Jul 8 2020, 1:36 PM.

Details

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.

Diff Detail

Event Timeline

tra created this revision.Jul 8 2020, 1:36 PM
tra updated this revision to Diff 276592.Jul 8 2020, 4:07 PM
tra edited the summary of this revision. (Show Details)

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

hfinkel added inline comments.
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 marked an inline comment as done.Jul 13 2020, 7:23 PM
tra added inline comments.
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 updated this revision to Diff 277639.Jul 13 2020, 7:27 PM

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

hfinkel added inline comments.Jul 13 2020, 7:29 PM
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 updated this revision to Diff 277887.Jul 14 2020, 10:05 AM

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

tra marked an inline comment as done.Jul 14 2020, 10:08 AM
tra added inline comments.
llvm/lib/MC/MCExpr.cpp
71

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

ABataev added inline comments.Jul 14 2020, 10:15 AM
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 marked an inline comment as done.Jul 14 2020, 11:17 AM
tra added inline comments.
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 added inline comments.Jul 14 2020, 11:20 AM
llvm/lib/MC/MCExpr.cpp
71

Hm, did you try to emit it as hexadecimal?

hfinkel added inline comments.Jul 14 2020, 12:15 PM
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 marked an inline comment as done.Jul 14 2020, 12:19 PM
tra added inline comments.
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 added inline comments.Jul 14 2020, 12:26 PM
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 marked an inline comment as done.Jul 14 2020, 12:29 PM
tra added inline comments.
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 added inline comments.Jul 14 2020, 12:35 PM
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 added inline comments.Jul 14 2020, 12:38 PM
llvm/lib/MC/MCExpr.cpp
71

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

ABataev added inline comments.Jul 14 2020, 12:41 PM
llvm/lib/MC/MCExpr.cpp
71

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

tra marked an inline comment as done.Jul 14 2020, 12:42 PM
tra added inline comments.
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 added inline comments.Jul 14 2020, 12:55 PM
llvm/lib/MC/MCExpr.cpp
71

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

tra updated this revision to Diff 277956.Jul 14 2020, 1:11 PM

Print negative values as hex.

tra marked an inline comment as done.Jul 14 2020, 1:11 PM
tra added inline comments.
llvm/lib/MC/MCExpr.cpp
71

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

Good idea. Done.

tra edited the summary of this revision. (Show Details)Jul 20 2020, 11:02 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2020, 9:19 PM
This revision was automatically updated to reflect the committed changes.