[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#706552, @junbuml wrote:

> > 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.
>
> Yes, I agree that the analysis shouldn't have to know about 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.
>
> If the main concern is the visibility of SelectionDAGBuilder in the base class for the analysis (SwitchLoweringCaseClusterBuilder in my current patch), don't you think further refactoring to remove SelectionDAGBuilder in SwitchLoweringCaseClusterBuilder by moving SwitchLoweringCaseClusterBuilderForDAG into SelectionDAGBuilder  could be reasonable design?


Even if moving the subclass into SelectionDAGBuilder, it still doesn't seem like a very nice design; the analysis isn't just an analysis if it's calling back into lowering code. Also, SelectionDAGBuilders details are currently leaking into the CaseCluster struct in the JT/BTCasesIndex member.

I think it would be better if the analysis could work independently: taking as input a set of cases and returning a vector of clustered cases (each cluster might simply consist of a kind and iterators/indexes into the original set of cases).


https://reviews.llvm.org/D31080