[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.

Mostly minor comments.



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:373
+enum OperatorChain {
+  OpChainNone,    ///< There is no operator.
+  OpChainOr,      ///< There are only ORs.
----------------
Normally I've seen LLVM prefixes these enums as `OC_OpChainNone` etc.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:420
+      // Given the previous operator, compute the current operator chain status.
+      OperatorChain CChain = OpChainNone;
+      switch (OCS) {
----------------
I'd leave this uninitialized here.  That way if you forget to assign to it down one path, sanitizers / compiler warnings will catch the bug.

I'm also not sold on the `OCS` and `CChain` names -- how about `ParentChain` and `NewChain`?


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:431
+        break;
+      default:
+        CChain = OpChainMixed;
----------------
I'd rather have this last case be:

```
case OpChainMixed:
  CChain = OpChainMixed;
  break;
```

That will make this switch "fully covered" and have the compiler issue a warning if someone adds a new enum but forgets to update the switch.



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:453
+        // operand(1).
+        OCS = CChain;
+        if (Value *RHS =
----------------
This style is a bit awkward, IMO.  (In a later change, no need to pre-commit review) do you mind changing `FindLIVLoopCondition` to return `std::pair<Value *, OperatorChain>` as well?


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:679
+          auto CaseVal = SI->findCaseValue(AllZero);
+          if (CaseVal != SI->case_default() &&
+              !BranchesInfo.isUnswitched(SI, AllZero)) {
----------------
Why do you care about the case being selected not being the default?


================
Comment at: test/Transforms/LoopUnswitch/basictest.ll:186
+
+; Make sure we dont unswitch, as we can not find a input value %a
+; that will effectively unswitch 0 or 3 out of the loop.
----------------
s/dont/don't/
s/a input/an input/


https://reviews.llvm.org/D29107