This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf
ClosedPublic

Authored by foad on May 13 2022, 9:11 AM.

Details

Summary

Most clients only used these methods because they wanted to be able to
extend or truncate to the same bit width (which is a no-op). Now that
the standard zext, sext and trunc allow this, there is no reason to use
the OrSelf versions.

The OrSelf versions additionally have the strange behaviour of allowing
extending to a *smaller* width, or truncating to a *larger* width, which
are also treated as no-ops. A small amount of client code relied on this
(ConstantRange::castOp and MicrosoftCXXNameMangler::mangleNumber) and
needed rewriting.

Diff Detail

Event Timeline

foad created this revision.May 13 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
foad requested review of this revision.May 13 2022, 9:11 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 13 2022, 9:11 AM
efriedma added inline comments.
llvm/lib/IR/ConstantRange.cpp
724

Making the bitwidth of the result here not equal to ResultBitWidth seems suspect.

I think there should just be an if (ResultBitWidth < BW) return getFull(ResultBitWidth); here. Then a simple conversion just works.

efriedma added inline comments.May 13 2022, 5:09 PM
llvm/lib/IR/ConstantRange.cpp
724

Actually, looking at D27294 again, maybe it is actually making the result bitwidth intentionally inflate like this.

This could use a comment explaining what it's doing, in any case.

lattner accepted this revision.May 13 2022, 10:53 PM

nice cleanup!

llvm/lib/Analysis/ConstantFolding.cpp
2884

I think this can be a zext given the top bit will be zero

llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp
138

plz drop the extraneous 'const' while here.

llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp
1220

Here too :)

This revision is now accepted and ready to land.May 13 2022, 10:53 PM
foad updated this revision to Diff 429466.May 14 2022, 10:02 AM

Address some review comments.

foad marked 2 inline comments as done.May 14 2022, 10:06 AM
foad added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
2884

Sure the first one could be zext, but the second one can't be, so it feels conceptually simpler (to me) to keep them both as sext.

llvm/lib/IR/ConstantRange.cpp
724

I agree it could use a comment but I don't feel qualified to write it - I am just trying to preserve the current behaviour.

martong removed a subscriber: martong.May 15 2022, 11:42 PM
foad added inline comments.May 17 2022, 4:34 AM
llvm/lib/IR/ConstantRange.cpp
724

@efriedma do you have any objection to the patch as-is?

Coming into this late, but I'd have preferred to see this separated into at least two pieces. One for each "non-obvious" adjustment, and one final one which just did the replace on the renaming sites. This differs from feedback from other reviewers above, so don't feel bound by this in any way. Just expressing the general preference.

efriedma added inline comments.May 17 2022, 12:27 PM
llvm/lib/IR/ConstantRange.cpp
724

No objection.

lattner added inline comments.May 17 2022, 4:48 PM
llvm/lib/Analysis/ConstantFolding.cpp
2884

likewise, I'm fine with this either way. zext is slightly more "Strength reduced" than sext, but it doesn't matter.