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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers 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);
 
----------------
hans wrote:
> 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.
yep, sorry I missed that.


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

https://reviews.llvm.org/D68131