[PATCH] D31080: [DAG] Extract switch lowering as a spearate object NFC

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In https://reviews.llvm.org/D31080#706390, @junbuml wrote:

> > Rather than splitting those out as virtual calls, I think the nicest design would be if the case clustering logic could work as an analysis: you feed it an array of cases, from which it computes some kind of result which indicates what cases go in jump tables, which are bit tests, etc. The SDAGBuilder would then consume that result to actually build the jump tables etc.
>
> Could you give me little bit more details about what you mention because for me it seems almost same as what current implementation is doing. If I understand correctly, current implementation of switch lowering use CaseClusterVector (a vector of CaseCluster) storing what cases go to JT and what cases go to BTest. In visitSwitch(), we first build CaseClusterVector without actual lowering. After then SDAGBuilder use CaseClusterVector to actually lower to JT or BTest. Did you mean for us to use another array instead of CaseClusterVector ?


The current implementation does try to split the analysis and lowering, but it doesn't succeed completely.

For example, findJumpTables() calls buildJumpTable() which creates a new MachineBasicBlock, creates a JumpTableHeader that it adds to the MachineFunction, and so on. In other words, it performs some lowering.

Your patch works around this by providing virtual methods for buildJumpTable(): one that doesn't actually build a jump table, and the other that does, which means it has to depend on SelectionDAGBuilder.

I'm saying it would be nice if the analysis and lowering could be separated further, so that the analysis does not have to know about SelectionDAGBuilder at all, and SelectionDAGBuilder would do the lowering entirely based on the results of the analysis.


https://reviews.llvm.org/D31080