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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:69
+    else if (MAI && !MAI->supportsSignedData())
+      OS << static_cast<uint64_t>(Value);
     else
----------------
ABataev wrote:
> tra wrote:
> > ABataev wrote:
> > > tra wrote:
> > > > hfinkel wrote:
> > > > > tra wrote:
> > > > > > hfinkel wrote:
> > > > > > > Will uint64_t always be correct here? Shouldn't this depend on SizeInBytes (like the hex printing does)?
> > > > > > 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);
> > > > > > ```
> > > > > 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)?
> > > > > 
> > > > Got it. I've masked out the unwanted bits. 
> > > Does cuda gdb correctly handle the bitfields with this fix? Can you read/write values to/from bitfield in the debugger?
> > 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
> > 
> > 
> > ```
> Hm, did you try to emit it as hexadecimal?
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83423