[PATCH] D29107: Fix a bug when unswitching on partial LIV for SwitchInst

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Comments inline.



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:372
+/// Operator chain lattice.
+enum OperatorChainTy {
+  OpChainNone,    ///< There is no operator.
----------------
Why not drop the `Ty` and call it a `OperatorChain`?  I've only seen `Ty` used in templated code when it isn't 100% obvious that something is a type.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:409
+  // Walk up the operator chain to find partial invariant conditions.
+  if (OCS)
+    if (BinaryOperator *BO = dyn_cast<BinaryOperator>(Cond))
----------------
Please put braces around such a large if block to make its extent obvious.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:439
+          if (Value *LHS =
+                  FindLIVLoopCondition(BO->getOperand(0), L, Changed, OCS, Cache)) {
+            Cache[Cond] = LHS;
----------------
Doesn't this regress the callers of `FindLIVLoopCondition` that pass in `nullptr` for `OCS`?  They used to get the recursive search before, and now they don't?

My preference would be to avoid allowing a null `OCS`, and instead have it be a `OperatorChainTy &` instead.  Callers that don't need it can pass in a dummy.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:684
+          // Dropping in a 0 to the chain will unswitch out the 0-casevalue.
+          auto AllZero = cast<ConstantInt>(Constant::getNullValue(SC->getType()));
+          auto CaseVal = SI->findCaseValue(AllZero);
----------------
`auto *`


https://reviews.llvm.org/D29107