This is an archive of the discontinued LLVM Phabricator instance.

[IntegerDivision][AMDGPU] Use CreateLogicalOr to block poison propagation.
ClosedPublic

Authored by craig.topper on Jul 27 2022, 9:50 PM.

Details

Summary

There are two ctlz intrinsics here with the zero_is_poison flag
set. There are also two comparisons that check if either of the
inputs the ctlzs are zero. We need to use a logical or to block
the poison from the ctlz if either of the inputs is zero.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 27 2022, 9:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 9:50 PM
craig.topper requested review of this revision.Jul 27 2022, 9:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 9:50 PM
Herald added a subscriber: wdng. · View Herald Transcript
nikic added inline comments.Jul 28 2022, 12:33 AM
llvm/lib/Transforms/Utils/IntegerDivision.cpp
217

I think you actually need to freeze the inputs here. Otherwise an undef value could be picked to be non-zero here and zero in the ctlz call.

nikic added inline comments.Jul 29 2022, 12:44 AM
llvm/lib/Transforms/Utils/IntegerDivision.cpp
233

I don't think we need this logical or, because both inputs are non-poison with the freeze.

238

We *do* need this one, because Ret0_4 may be poison (through SR).

241

We also need this one, because RetDividend may be poison (through SR).

aqjune added inline comments.Aug 3 2022, 9:22 AM
llvm/lib/Transforms/Utils/IntegerDivision.cpp
136

Should Dividend and Divisor here be frozen to address the comment?

craig.topper added inline comments.Aug 3 2022, 9:57 AM
llvm/lib/Transforms/Utils/IntegerDivision.cpp
136

I don't understand the question. I froze them on line 256 and 257. Do you think they should be frozen earlier int he function or in a different basic block?

arsenm added inline comments.Aug 3 2022, 9:59 AM
llvm/test/CodeGen/AMDGPU/srem64.ll
1023–1028

These are all small improvements

nikic added a comment.Aug 3 2022, 10:00 AM

There are some failing unit tests.

aqjune added inline comments.Aug 3 2022, 5:42 PM
llvm/lib/Transforms/Utils/IntegerDivision.cpp
136

Oops, I missed the updates at 256, 257, sorry for this.

Change one CreateLogicalOr back to CreateOr.
Update unit test I missed before

aqjune accepted this revision.Aug 4 2022, 11:35 PM
This revision is now accepted and ready to land.Aug 4 2022, 11:35 PM
arsenm accepted this revision.Sep 15 2022, 8:22 AM
This revision was landed with ongoing or failed builds.Sep 15 2022, 9:58 AM
This revision was automatically updated to reflect the committed changes.