[PATCH] D45439: [IRCE] Use NUW flag for indvar
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 10 03:51:23 PDT 2018
mkazantsev added inline comments.
================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:928
// not wrap. This restriction can potentially be lifted in the future.
+ if (HasNoSignedWrap(AR) ||
+ (!IsSigned && AR->hasNoUnsignedWrap())) {
----------------
Why not `(Signed && HasNoSignedWrap(AR)) || (!IsSigned && AR->hasNoUnsignedWrap())`?
================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
- if (!HasNoSignedWrap(AR))
- return false;
----------------
Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)
================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
+ if (HasNoSignedWrap(AR) ||
+ (!IsSigned && AR->hasNoUnsignedWrap())) {
----------------
mkazantsev wrote:
> Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)
If you look at `HasNoSignedWrap`, it has a bit more complex check than just looking into the immediate flag, it also tries to go through `sext` to find this flag. Can we do the same for `nuw` and `zext` to keep things consistent?
================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:964
// Try to turn eq/ne predicates to those we can work with.
- if (Pred == ICmpInst::ICMP_NE && LatchBrExitIdx == 1)
+ if (Pred == ICmpInst::ICMP_NE && LatchBrExitIdx == 1) {
// while (++i != len) { while (++i < len) {
----------------
It seems to be an independent change, please commit it separately as NFC.
================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:328
+; CHECK-LABEL: signed_var_imm_dec_eq
define void @signed_var_imm_dec_eq(i32* nocapture %a, i32* nocapture readonly %b, i32* nocapture readonly %c, i32 %M) {
----------------
Same, this can go as NFC without approval.
================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:361
+
+; CHECK-LABEL: test_inc_eq_var_nuw(
+; CHECK: main.exit.selector:
----------------
Please add `CHECK` or `CHECK-NOT` on existence/non-existence of preloop and postloop, so that we can make sure that IRCE does good job generating/not generating them.
Also the same applies to the tests above, you can add these checks as NFC separately (not super-urgent, but useful).
https://reviews.llvm.org/D45439
More information about the llvm-commits
mailing list