[PATCH] D21291: [SimplifyCFG] Range reduce switches

[James Molloy via llvm-commits]

jmolloy added a comment.

Hi guys,

Thanks for the comments. I was away on vacation for 2 weeks which is why this has been sitting around.

Hans, if you're OK with it being done in DAG for the reasons mentioned, is this good to commit?

Cheers,

James


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8434
@@ -8358,3 +8433,3 @@
       // Try building a partition from Clusters[i..j].
       if (isDense(Clusters, &TotalCases[0], i, j, MinDensity)) {
         unsigned NumPartitions = 1 + (j == N - 1 ? 0 : MinPartitions[j + 1]);
----------------
eli.friedman wrote:
> hans wrote:
> > hans wrote:
> > > This code doesn't take into account that a range of cases could be transformed to become dense, so I'm not sure your patch will actually find any more jump table lowerings that don't cover the whole switch?
> > I guess what I was saying with this comment is that since you're not going to get the benefit of transforming parts of the switch, it there a point in doing this in the DAG instead of in the IR? I think the rotation trick should work in the IR too..
> Err, I guess the rotation trick works in IR?  You can let switch lowering do the compare+branch.  Not sure what I was thinking before.
> 
> That said, we don't really want to do switch lowering in IR. I mean, in theory, we can lower switches in IR, but we want to do all of switch lowering at the same time.  Otherwise, you end up with weird guessing games to predict what SelectionDAG will actually do.
I agree with Eli here. It'd be nice to densify individual switch case ranges, and perhaps we could do that in the future?

The main advantage I got from moving from IR to SDAG for this is the heuristics don't need second-guessing, which was a major improvement. Doing this in IR would give one large advantage - it'd allow the switch table lookup lowering to work too. I intend to implement this directly in the switch table lookup lowering code though, which should be fairly simple.


Repository:
  rL LLVM

http://reviews.llvm.org/D21291