This is an archive of the discontinued LLVM Phabricator instance.

[KnownBits] Return `0` for poison {s,u}div inputs
ClosedPublic

Authored by goldstein.w.n on May 18 2023, 5:38 PM.

Details

Summary

It seems consistent to always return zero for known poison rather than
varying the value. We do the same elsewhere.

Diff Detail

Event Timeline

goldstein.w.n created this revision.May 18 2023, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 5:38 PM
goldstein.w.n requested review of this revision.May 18 2023, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 5:38 PM
goldstein.w.n added a reviewer: luke.
goldstein.w.n added inline comments.
llvm/test/CodeGen/WebAssembly/pr59626.ll
18

I think this is a meaningless regression. The IR being tests evaluates to just ret i8 poison @luke is this okay?

luke added inline comments.May 19 2023, 4:34 AM
llvm/test/CodeGen/WebAssembly/pr59626.ll
18

Can you point to the IR test that's evaluating to just poison? As long as this test case isn't crashing then I'm ok, but I'm probably not the right person to ask about KnownBits. I'm surprised this is the only test case that is affected!

goldstein.w.n added inline comments.May 19 2023, 8:08 AM
llvm/test/CodeGen/WebAssembly/pr59626.ll
18

Can you point to the IR test that's evaluating to just poison?

This test @f:
https://alive2.llvm.org/ce/z/VDJkEb

As long as this test case isn't crashing then I'm ok, but I'm probably not the right person to ask about KnownBits. I'm surprised this is the only test case that is affected!

I'm just asking if the affects to the wasm codegen is okay.

All do INT_MIN / -1 case

nikic added a comment.EditedMay 19 2023, 12:30 PM

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

goldstein.w.n added a comment.EditedMay 19 2023, 1:54 PM

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Actually testing the impl without this, the zero, zero check will still be needed (just
later on) so would prefer to keep that regardless.

nikic added a comment.May 19 2023, 2:04 PM

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Is this referring to the code in D150923? That is we do the early isZero() check here and then divComputeLowBit() from that patch has an implicit precondition that the denominator is not zero? Adding an explicit < BitWidth check there seems more obvious to me, unless it's more than that?

IMHO we shouldn't go out of the way to handle such cases -- if we need to handle this for other reasons (e.g. for shift amounts, or for conflicts) that's one thing, but explicitly checking for poison result for known constant cases is pretty pointless -- those will get folded to actual poison values by code that can do so.

The INT_MIN / -1 case isn't necessary, but the LHS.isZero() || RHS.isZero() cases reduce the number of
checks we need elsewhere (specifically now we know MinTZ < BitWidth)

Edit: But I have no particularly strong feelings either way. the Zero, Zero case just
seemed to be the simplest way to handle it. The INT_MIN, -1 case was just to
be consistent. If you feel either/both should be dropped I have no objections.

Is this referring to the code in D150923? That is we do the early isZero() check here and then divComputeLowBit() from that patch has an implicit precondition that the denominator is not zero? Adding an explicit < BitWidth check there seems more obvious to me, unless it's more than that?

Need multiple (although I could just move this isZero() checks to divComputeLowBit.

Remove cases that don't help cleanup rest of the code

nikic accepted this revision.Jun 6 2023, 5:33 AM

LGTM

llvm/lib/Support/KnownBits.cpp
754

nit: Poison -> UB

This revision is now accepted and ready to land.Jun 6 2023, 5:33 AM
This revision was automatically updated to reflect the committed changes.