[PATCH] D151208: [KnownBits] Add support for nuw/nsw on shifts
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 24 02:40:08 PDT 2023
nikic added inline comments.
================
Comment at: llvm/lib/Support/KnownBits.cpp:176
+ Known.One = LHS.One << ShiftAmt;
+ if ((NUW || NSW) && ShiftAmt != 0) {
+ KnownBits ShiftedOutBits = LHS.extractBits(ShiftAmt, BitWidth - ShiftAmt);
----------------
goldstein.w.n wrote:
> nit style: Instead of nesting if statements, can you invert them and just return immediately? Think it makes code easier to read.
I've done this for the first check here and also for the isUnknown checks in https://github.com/llvm/llvm-project/commit/2d48a771fc00681414c54ea3936bff91f3b253c4.
================
Comment at: llvm/lib/Support/KnownBits.cpp:227
// possible shifts.
APInt MaxShiftAmount = RHS.getMaxValue();
+ if (!LHS.isUnknown() || (NUW && NSW)) {
----------------
goldstein.w.n wrote:
> in the NSW/NUW we should be able to further bound MaxShiftAmount with LHS.countLeadingZeros no? (and poison if MinShiftAmunt > LHS.countLeadingZeros)
Yes, but this is already implicitly handled by the loop below. It stops once it gets the first shift amount that results in poison due to nuw/nsw.
================
Comment at: llvm/lib/Support/KnownBits.cpp:250
}
Known.Zero.setLowBits(MinTrailingZeros);
----------------
goldstein.w.n wrote:
> can you assert no conflict? Its not super obvious just reading the code that the loop will be enough to clear the all zeros/ones initial state in Known.
You are right, it's necessary to check for conflicts here. I've added a general assertion for this in the exhaustive tests, so we don't have to remember checking it for every function: https://github.com/llvm/llvm-project/commit/d4bfc144eaa6e34eafaab1e8574421ebd2bb78ed
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151208/new/
https://reviews.llvm.org/D151208
More information about the llvm-commits
mailing list