[PATCH] D71620: [Attributor][WIP] AAValueConstantRange: Value range analysis using constant range

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Sorry for the wait. I wanted to do a proper initial review and I didn't find the right time.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1352
 
+/// State for a integer range.
+struct IntegerRangeState : public AbstractState {
----------------
Typo: 'an'

Below, can we make the comments start with three '/'?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1421
+    Known = Known.intersectWith(R);
+  }
+
----------------
I think you need to adjust Assumed too otherwise it might not be a proper subset of Known anymore.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1440
+    unionAssumed(R);
+  }
+
----------------
Do we need to intersect the known range here as well?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2590
+                BasicBlock *NewDeadBB =
+                    SplitBlock(BB, II, nullptr, nullptr, nullptr, ".i2c");
                 assert(isa<BranchInst>(BB->getTerminator()) &&
----------------
I formated the file yesterday, rebase and this should be gone.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4963
+      else {
+        // TODO: Replace value to undef if range is empty(=undef).
+      }
----------------
If you pass the Attributor `changeUseAfterManifest` should make this simple.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4968
+    return false;
+  }
+
----------------
Can we pass the constant range arguments as const references in the various places.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4975
+    if (AssumedConstantRange.isFullSet())
+      return ChangeStatus::UNCHANGED;
+
----------------
Shouldn't this imply "an invalid state" (= the worst possible state)? If so, it should never be make it to this point (which you should assert instead).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4991
+                          << *C << "\n");
+        replaceAllInstructionUsesWith(V, *C);
+        Changed = ChangeStatus::CHANGED;
----------------
You should use one of the Attributor::doSthAfterManifest helpers (or a new one). We should avoid modifying values/uses while in the manifest stage.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5079
+                      << " is initialized to " << getState());
+  }
+
----------------
We should probably provide a helper to make sure the type of a value is OK (I guess integer) and check it in the initialize.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5092
+          if (!LHS->getType()->isIntegerTy() || !RHS->getType()->isIntegerTy())
+            return false;
+
----------------
With a check in the initializers this would go away.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5129
+                  }
+              }
+            T.indicatePessimisticFixpoint();
----------------
I don't think we need to look at all loops here.
The problem is also that `getSCEVAtScope` might silently fail to actually compute the exit value and instead continue computation with the original one.

I think this is OK already: `SE->getUnsignedRange(SE->getSCEV())`




================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5131
+            T.indicatePessimisticFixpoint();
+            return false;
+          }
----------------
Style: With early exits we can minimize indention:
`if (!SE || !LI || !I) return false;`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5132
+            return false;
+          }
+
----------------
We should consider putting some of this in helper methods.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5143
+
+          return true;
+        } else if (auto *CmpI = dyn_cast<CmpInst>(I)) {
----------------
Add a TODO to track the known state as well.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5212
+        return T.isValidState();
+      }
+    };
----------------
Can you move this case to the beginning `Inst * I = ...; if (!I) { ... }`, so the rest has one level less of indention?



================
Comment at: llvm/test/Transforms/Attributor/value-simplify.ll:193
+; CHECK-NEXT:    [[R:%.*]] = call i32 @ipccp3i(i32 7)
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
----------------
Please add a FIXME here, should be 7.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71620/new/

https://reviews.llvm.org/D71620