This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Simplify and generalize constant printer.
ClosedPublic

Authored by tra on Sep 8 2021, 11:47 AM.

Diff Detail

Event Timeline

tra created this revision.Sep 8 2021, 11:47 AM
tra requested review of this revision.Sep 8 2021, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2021, 11:47 AM

Is it a pain to add a test? I know the reproducer from the bug still crashes llvm, not sure if it's hard to reproduce the i128 issue independently.

tra updated this revision to Diff 371417.Sep 8 2021, 12:28 PM
tra edited the summary of this revision. (Show Details)

Added a test

jlebar accepted this revision.Sep 8 2021, 12:39 PM
jlebar added inline comments.
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1764–1766

Any reason we have this restriction on 16 bytes long? Like, it'll work for i128 but fail for i256, right?

Could we use InlinedVector instead of a fixed std::array?

This revision is now accepted and ready to land.Sep 8 2021, 12:39 PM
tra updated this revision to Diff 371428.Sep 8 2021, 1:07 PM

Use SmallVector

llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1764–1766

InlinedVector is an absl thing and is not available in LLVM.
Updated to use SmallVector instead.

tra added inline comments.Sep 8 2021, 1:50 PM
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1756–1758

This (and the original code) looks suspicious. Why would we allow filling in more bytes than we were told to by the caller?
I think this should've been an assert(AllocSize <= Bytes); addZeroes(Bytes);

Or it was a typo and the code should've read if (s > Bytes) s = Bytes essentially only filling up to Bytes, but no more.

tra updated this revision to Diff 371468.Sep 8 2021, 4:06 PM

Figured out what's up with zero-filling code.

tra updated this revision to Diff 371472.Sep 8 2021, 4:31 PM

One more test for zero-filling of alignment gaps.

This revision was landed with ongoing or failed builds.Sep 9 2021, 11:33 AM
This revision was automatically updated to reflect the committed changes.