[PATCH] D68131: Switch lowering: omit range check for bit tests when default is unreachable (PR43129)

Tue Oct 1 12:30:56 PDT 2019

hans marked 2 inline comments as done.
hans added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2650
   B.Reg = FuncInfo.CreateReg(B.RegVT);
   SDValue CopyTo = DAG.getCopyToReg(getControlRoot(), dl, B.Reg, Sub);

----------------
nickdesaulniers wrote:
> So it seems to me that `CopyTo` might not even end up as the root.  It looks like we have 3 cases that affect the root:
> 1. `!B.OmitRangeCheck`
> 2. `MBB != NextBlock(SwitchBB)`
> 3. Neither 1 or 2
> 
> For cases 1 and 2, we do all this wasteful work to create `CopyTo` and `Sub`, just to throw it away.  It seems like `RangeSub` isn't needed for case 2.  I think it would be clearer and do less work for each case to have a simple 3 arm conditional, rather than successive conditionals that may just throw away previous work to reassign to `Root`.
Note that Root below isn't just assigned to, it's also passed as an argument when creating BRCOND or the BR node.

So in the cases where CopyTo doesn't end up being the root, it's been passed as the chain element to one of those instructions. In all cases, it is on the chain -- it's never thrown away. (And neither is RangeSub since it's an input to CopyTo.)

I realize that's maybe not super clear, but I think this style of updating the selection dag root by using the old root as the chain of the new root is common.

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

https://reviews.llvm.org/D68131