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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with minor comments.



================
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) {
----------------
yamauchi wrote:
> sanjoy wrote:
> > 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.
> No, this logic does not subsume the logic in line 1431 (I verified that by commenting it out and running the tests.) This is because the 1431 logic handles the cases where 'Condition' is an operand of 'Val' whereas this logic handles the cases where 'Condition' is *not* an operand of 'Val', but one ('Op') of the operands of 'Val' can be inferred through the value of 'Condition'. Note this logic calls getValueFromCondition for 'Op'.
> 
> I moved the 1431 logic here and combined them.
> 
> I also factored out some common logic out to constantFoldUser() and simplified its call sites.
> 
> 
Ok, SGTM.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1488
+    Value* Condition = SI->getCondition();
+    if (!isa<IntegerType>(Val->getType()))
       return false;
----------------
yamauchi wrote:
> sanjoy wrote:
> > I think this can be an assert -- switch only allows integer conditions.
> I assume 'Val' can be any value live through the switch for which we're trying to find its value on this edge, and can be of any type?
Yes, you're right.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:152
 
+  Optional<APInt> getConstantInteger() const {
+    Optional<APInt> Result;
----------------
I didn't notice this the first time around, but I'd calls this function `asConstantInteger` (or something else that does not start with `get`).  The `getXXX` in this class are simple accessors (which this function is not) so it is useful to differentiate this.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:154
+    Optional<APInt> Result;
+    if (isConstant() && isa<ConstantInt>(Val)) {
+      Result = Val->getUniqueInteger();
----------------
I think we can have direct returns here, instead of assigning to `Result` and then `return Result;`.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1385
+    assert(CI->getOperand(0) == Op && "Operand 0 isn't Op");
+    if (ConstantInt *C = dyn_cast_or_null<ConstantInt>(
+            SimplifyCastInst(CI->getOpcode(), OpConst,
----------------
This is minor, but LLVM tends to use `auto *` for assignments whose types are obvious from RHS.


================
Comment at: lib/Analysis/LazyValueInfo.cpp:1463
+              Optional<APInt> OpConst = OpLatticeVal.getConstantInteger();
+              if (OpConst.hasValue()) {
+                Result = constantFoldUser(Val, Op, OpConst.getValue(), DL);
----------------
I suspect you can do

```
if (Optional<APInt> OpConst = OpLatticeVal.getConstantInteger())
```



================
Comment at: lib/Analysis/LazyValueInfo.cpp:1479
   if (SwitchInst *SI = dyn_cast<SwitchInst>(BBFrom->getTerminator())) {
-    if (SI->getCondition() != Val)
+    Value* Condition = SI->getCondition();
+    if (!isa<IntegerType>(Val->getType()))
----------------
The spacing is still off here, clang-format should fix this.


https://reviews.llvm.org/D34822