Page MenuHomePhabricator

[ADT] Remove 0-width Asserts in APInt.getZExtValue
ClosedPublic

Authored by seldridge on Nov 29 2021, 10:07 PM.

Details

Summary

Remove assertion that disallows getting a zero-extended value from a
zero-width APInt. This check is too restrictive and makes it difficult
to use APInt to model zero-width things, e.g., zero-width wires in the
CIRCT project.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Diff Detail

Event Timeline

seldridge created this revision.Nov 29 2021, 10:07 PM
seldridge published this revision for review.Nov 29 2021, 10:37 PM

This diff removes restrictions around zero/sign extension of zero-width APInts. Specifically, there is a restriction introduced in https://reviews.llvm.org/D109555 that disallows zero extension of zero-width APInts. Additionally, there are some old asserts inside MathExtras, added in https://reviews.llvm.org/D22442, which assert on sign extension of any zero-width number (which are used pervasively inside APInt code to implement its sign extension). This is attempting to better enable the CIRCT project to rely on APInt to represent zero-width wires without introducing lots of additional guards to work around APInt methods which assert when used. More information on what specifically this is attempting to solve for CIRCT is here: https://github.com/llvm/circt/issues/2255

Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 10:37 PM
seldridge updated this revision to Diff 390706.Nov 30 2021, 6:55 AM

Run clang-format

Run clang-format to squash trailing whitespace.

darthscsi accepted this revision.Nov 30 2021, 10:01 AM
darthscsi added a subscriber: darthscsi.

LGTM.

This revision is now accepted and ready to land.Nov 30 2021, 10:01 AM
nikic requested changes to this revision.Nov 30 2021, 10:07 AM
nikic added a subscriber: nikic.

Based on the discussion in D109555, the current consensus seems to be that while zext of zero-width is well-defined, sext is not.

This revision now requires changes to proceed.Nov 30 2021, 10:07 AM
nikic added a reviewer: foad.Nov 30 2021, 10:09 AM
uenoku added a subscriber: uenoku.Nov 30 2021, 10:18 AM

Drop SExt commit. This is somewhat controversial and, after further
examination on my end, seemingly not needed. I can revive this if it
becomes necessary for CIRCT.

Can you please add a test to TEST(APIntTest, ZeroWidth)?

lattner accepted this revision.Nov 30 2021, 12:05 PM

This revised change to getZExtValue only LGTM, because it is the only reasonable definition of this. As @nikic points out, we need to be more deliberate about bigger changes: defining a zero width thing to always have zero value isn't obviously correct.

Please change the commit message to "Remove zero-width assertion from getZExtValue" to make it more specific, thanks!

Add test of zero-width ZExt behavior.

foad added a comment.Nov 30 2021, 12:42 PM

LGTM too, thanks!

seldridge updated this revision to Diff 390803.Nov 30 2021, 1:09 PM
seldridge retitled this revision from [ADT][Support] Remove zero-width assertions to [ADT] Remove 0-width Asserts in APInt.getZExtValue.
seldridge edited the summary of this revision. (Show Details)

Update commit message/diff summary to reflect reduced scope.

nikic accepted this revision.Nov 30 2021, 1:35 PM

LG

llvm/unittests/ADT/APIntTest.cpp
3057

Zero extension :)

This revision is now accepted and ready to land.Nov 30 2021, 1:35 PM
seldridge updated this revision to Diff 390811.Nov 30 2021, 1:47 PM

Fix comment. :)

This revision was landed with ongoing or failed builds.Nov 30 2021, 2:03 PM
This revision was automatically updated to reflect the committed changes.
seldridge marked an inline comment as done.