Details
Diff Detail
Event Timeline
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. |
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() |
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. |
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
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
---|---|---|
2069 | Thanks, I delete the nuw case. |
Negator should not be inspecting users. You need to pass through the nsw flag from the caller to handle it this way.
pass through the nsw flag from the caller to avoid inspecting users according comment
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp | ||
---|---|---|
235–242 ↗ | (On Diff #549227) | I think checking hasOneUse twice here is somewhat confusing |
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
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.