Merge more Select pseudo instructions in emitSelect() by allowing other instructions between them as long as they do not clobber CC.
Details
Diff Detail
Event Timeline
Tried to make some more tests in multiselect.ll to cover merging of Select pseudos. Trunk fails on @test1.
At least at the moment, the DAG->DAG output/finalize-isel input is having the machine instructions in a corresponding order to the LLVM I/R ones (the MDBR in @test1 is really in between the two SelectF64 instructions). Would it be better to do .mir tests and just test the emitSelect() output by doing '-run-pass=finalize-isel'? That way we would have full control over input to emitSelect() and check its output directly.
Put 'NextMIIt->getOperand(3).getImm() == CCValid' in an assert instead since this must be true.
Avoid name clash with argument 'MI' by using the name 'SelMI' instead in for loop.
Save DBG_VALUE instructions that use a Select pseudo in a vector and move them to after the new PHI nodes. In docs/SourceLevelDebugging.rst, there is an example (under "Instruction Scheduling") of moving down a definition would require keeping the original DBG_VALUE instruction but setting it to undefined, while creating a new DBG_VALUE following the sunk instruction. This does not seem to be done by e.g. MachineSink, which merely moves the DBG_VALUE down...
debuginstr-02.mir test updated to also check that the DBG_VALUE instructions are moved (and not deleted).
This looks quite good to me, just a couple of (more or less cosmetic) comments inline.
lib/Target/SystemZ/SystemZISelLowering.cpp | ||
---|---|---|
6565–6566 | Comment needs to be updated. | |
6613 | Why do this here? I think it would be more straightforward for the caller to delete those, like it did before. (Or else, the function comment/name should be updated.) | |
6659 | It would seem clearer to check for isDebugValue outside the loop -- that characteristic is loop-invariant here. Then you wouldn't need the IsDbg flag either. |
Thanks for review - patch updated.
Now also checking for *any* debug instruction. Currently this would mean also recognizing a debug label, which I am not sure if might be present here, but it should not be using any Select pseudo value.
lib/Target/SystemZ/SystemZISelLowering.cpp | ||
---|---|---|
6613 | My idea was to only iterate over the Selects once, but I suppose that may make the call to createPHIsForSelects() less natural. | |
6659 | Good point, thanks. I also made sure that any debug instruction will not interfere with Count, while expecting that a debug instruction that uses a select must be a debug value (currently there could perhaps also be debug labels). |
Comment needs to be updated.