This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Propagate the nsw for instruction neg-sub
AbandonedPublic

Authored by Allen on Aug 5 2023, 2:36 AM.

Diff Detail

Event Timeline

Allen created this revision.Aug 5 2023, 2:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2023, 2:36 AM
Allen requested review of this revision.Aug 5 2023, 2:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2023, 2:36 AM
goldstein.w.n added inline comments.Aug 5 2023, 8:56 PM
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2069

I don't think its okay to modify NegOp1 inplace like this. the flag propagation works in the context of the transform at hand but might not be applicable to all users.

Also this desperately needs some comments.

Allen updated this revision to Diff 547650.Aug 6 2023, 11:22 PM
Allen edited the summary of this revision. (Show Details)

Add condition Op1->hasOneUse() and its related test src_neg_sub_both_nsw_other_use

Allen marked an inline comment as done.Aug 6 2023, 11:25 PM
Allen added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2069

Apply your comment, thanks. Also add a new test src_neg_sub_both_nsw_other_use for the new checking Op1->hasOneUse()

nikic requested changes to this revision.Aug 7 2023, 3:10 AM

You are making assumptions about the transforms Negator can do here -- in particular that a "sub" is only produced when negating another "sub". This is not correct. E.g. you miscompile this case: https://alive2.llvm.org/ce/z/Q_MiP4

You need to either integrate the flag handling into Negator to make sure it is only applied in the cases where it is actually valid, or you need to make this a separate transform.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2069

You don't seem to have test coverage for the nuw case, and it seems pretty useless to boot. sub nuw 0, %x can only be non-poison if %x is zero. This is not a useful case to handle.

This revision now requires changes to proceed.Aug 7 2023, 3:10 AM
Allen updated this revision to Diff 547754.Aug 7 2023, 6:03 AM
Allen marked an inline comment as done.
Allen retitled this revision from [InstCombine] Propagate the nuw/nsw for instruction neg-sub to [InstCombine] Propagate the nsw for instruction neg-sub.

address comments
1、don't propagate the nuw because it seems pretty useless to boot (sub nuw 0, %x can only be non-poison if %x is zero)
2、Reconstructed in negation according to suggestions
3、Add the new negative test neg_and_shl_sub

Allen marked an inline comment as done.Aug 7 2023, 6:06 AM
Allen added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
2069

Thanks, I delete the nuw case.

nikic requested changes to this revision.Aug 7 2023, 6:20 AM

Negator should not be inspecting users. You need to pass through the nsw flag from the caller to handle it this way.

This revision now requires changes to proceed.Aug 7 2023, 6:20 AM
Allen updated this revision to Diff 547778.Aug 7 2023, 7:24 AM
Allen marked an inline comment as done.

pass through the nsw flag from the caller to avoid inspecting users according comment

arsenm added inline comments.Aug 17 2023, 11:13 AM
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
235–251

I think checking hasOneUse twice here is somewhat confusing

Allen added inline comments.Aug 19 2023, 11:05 PM
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
235–251

Thanks @arsenm for your idea.
I double check the hasOneUse because it may be also here when the I->getOperand(0) is a const value.

nikic requested changes to this revision.Aug 21 2023, 8:17 AM

I'm going to implement this myself. We need something along these lines, just with full proofs and test coverage: https://gist.github.com/nikic/07bfd88af3607cdb4ba84bdaf299b07f

This revision now requires changes to proceed.Aug 21 2023, 8:17 AM
Allen added a comment.Aug 21 2023, 8:29 PM

That's great, thanks very much

Allen abandoned this revision.Aug 21 2023, 8:29 PM
nikic added a comment.Aug 22 2023, 6:45 AM

For the record, here's the new patch: D158510