This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Improve emitSelect()
ClosedPublic

Authored by jonpa on Sep 16 2019, 7:10 AM.

Details

Reviewers
uweigand
Summary

Merge more Select pseudo instructions in emitSelect() by allowing other instructions between them as long as they do not clobber CC.

Diff Detail

Event Timeline

jonpa created this revision.Sep 16 2019, 7:10 AM
jonpa updated this revision to Diff 220455.Sep 17 2019, 2:23 AM

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.

jonpa updated this revision to Diff 221483.Sep 24 2019, 1:58 AM

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.

jonpa updated this revision to Diff 221698.Sep 25 2019, 3:18 AM
jonpa marked 5 inline comments as done.

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).

uweigand accepted this revision.Sep 25 2019, 5:21 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 25 2019, 5:21 AM
jonpa closed this revision.Sep 25 2019, 7:00 AM

Thanks for review!
r372873