[PATCH] D34822: [LVI] Constant-propagate a zero extension of the switch condition value through case edges

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:152
 
+  bool getConstantInteger(APInt& Result) const {
+    if (isConstant() && isa<ConstantInt>(Val)) {
----------------
Can we return an `Optional<APInt>` here?


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1372
+static bool usesOperand(User* Usr, Value* Op) {
+  return std::find(Usr->op_begin(), Usr->op_end(), Op) != Usr->op_end();
+}
----------------
You can use `User::operands()` and `llvm::find` here.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1378
+// store the integer constant in Result. Otherwise, return false.
+static bool getConstantEdgeValue(Value* Val, Value* Op, const APInt& OpConstVal,
+                                 const DataLayout& DL, APInt& Result) {
----------------
I think we can return `Optional<APInt>` here.

Also: how about dropping `Edge` from the name, since we're not doing anything specific to bblock edges here?  How about `constantFoldUser`?


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1383
+  if (auto* CI = dyn_cast<CastInst>(Val)) {
+    assert(CI->getOperand(0) == Op && "Operand 0 isn't Condition");
+    if (ConstantInt* C = dyn_cast_or_null<ConstantInt>(
----------------
s/Condition/Op/


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1396
+    Value* LHS = Op0Match ? OpConst : BO->getOperand(0);
+    Value* RHS = Op1Match ? OpConst : BO->getOperand(1);
+    if (ConstantInt* C = dyn_cast_or_null<ConstantInt>(
----------------
In LLVM `*` aligns with the value.  clang-format should fix this.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1465
+      if (User* Usr = dyn_cast<User>(Val)) {
+        if (isa<IntegerType>(Val->getType())) {
+          for (unsigned i = 0; i < Usr->getNumOperands(); ++i) {
----------------
Does this logic subsume the logic in line 1431?  And can we write the switch case case (no pun intended! :) ) in the same way?  If so, I'd suggest pulling this logic out into a function and calling it once here and once from the switch-case block.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1488
+    Value* Condition = SI->getCondition();
+    if (!isa<IntegerType>(Val->getType()))
       return false;
----------------
I think this can be an assert -- switch only allows integer conditions.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1493
+      // Check if Val has Condition as an operand.
+      if (User* Usr = dyn_cast<User>(Val)) {
+        ValUsesCondition = usesOperand(Usr, Condition);
----------------
Usually LLVM code does not use `{` for single statement `if`s.


https://reviews.llvm.org/D34822