This is an archive of the discontinued LLVM Phabricator instance.

[PEI] Remove required properties and use 'if' instead of std::function
ClosedPublic

Authored by rnk on Oct 5 2017, 1:04 PM.

Details

Summary

After r303360, we initialize UsesCalleeSaves in runOnMachineFunction,
which runs after getRequiredProperties. UsesCalleeSaves was initialized
to 'false', so getRequiredProperties would always return an empty set.
We don't have a TargetMachine available early anymore after r303360.
Just removing the requirement of NoVRegs seems to make things work, so
let's do that.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Oct 5 2017, 1:04 PM
dschuff edited edge metadata.Oct 5 2017, 1:29 PM
dschuff added a subscriber: qcolombet.

Removing the requirement works fine now because the passes are set up correctly. The original intention behind getRequiredProperties in this case was that if the pass configuration were to be changed and PEI were to be called before regalloc on a target which wasn't prepared for that, there would be an easy assertion to say what went wrong.

We could just add a conditional assertion in runOnMachineFunction to ensure things are correct; in fact my original patch to PEI for WebAssembly more or less did that. But my reviewer (@qcolombet ) really wanted a stronger mechanism to enforce that kind of invariant and IIRC that's why getRequiredProperties was born.

I guess if we don't have the target anymore at the right time, we can't really properly set getRequiredProperties. I'd be fine with going back to a simple assertion here, but I guess that risks re-opening the debate :)

llvm/lib/CodeGen/PrologEpilogInserter.cpp
82 ↗(On Diff #117874)

regardless of what we decide about getRequiredProperties we should do the rest of this change (i.e. this and below). There was some discussion about it after it originally landed and it was my intention to do more or less exactly this part of your patch, and I didn't get around to it :(

MatzeB edited edge metadata.Oct 5 2017, 1:35 PM

+1. You could still check the condition in an assert() at the right place though.

rnk updated this revision to Diff 117880.Oct 5 2017, 1:43 PM
  • Try to add the right assert
MatzeB added inline comments.Oct 5 2017, 1:43 PM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
155 ↗(On Diff #117874)

I'd name the variable UsesPhysRegsForPEI.

174 ↗(On Diff #117874)

With the std::function gone we can rename doSpillCalleeSavedRegs back to spillCalleeSavedRegs

201–205 ↗(On Diff #117874)

This feels wrong, we should only clearVirtRegs() when we actually scavenged them. Checking the code it seems that the clearing is already done in scavengeFrameVirtualRegs() nowadays so we can remove it here. Maybe we should also skip the whole if (UsesCalleeSaves) then too as it seems target can already control this bit via TRI::requiresRegisterScavenging`.

dschuff added inline comments.Oct 5 2017, 1:51 PM
llvm/lib/CodeGen/PrologEpilogInserter.cpp
201–205 ↗(On Diff #117874)

+1 in general. I'm ambivalent on removing if (UsesCalleeSaves) altogether. It's true that no target I can imagine would have UsesCalleeSaves be false and requiresRegisterScavenging be true, but having it explicit makes it a bit more obvious. I'd be OK either way I think.

492 ↗(On Diff #117880)

I think this is the right assertion, but there should be a comment here about why we are doing it manually instead of using getRequiredProperties, since that would be the expected thing.

rnk marked 3 inline comments as done.Oct 5 2017, 1:53 PM
rnk added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
201–205 ↗(On Diff #117874)

I agree. Most targets don't need register scavenging, and wasm (the only target that uses vregs past this point) definitely doesn't need it either.

rnk updated this revision to Diff 117883.Oct 5 2017, 1:53 PM
  • more simplifications
MatzeB accepted this revision.Oct 5 2017, 1:56 PM
MatzeB added a reviewer: qcolombet.

LGTM; maybe wait a bit for @qcolombet to comment.

This revision is now accepted and ready to land.Oct 5 2017, 1:56 PM
MatzeB added a subscriber: thegameg.Oct 5 2017, 1:57 PM
rnk marked 2 inline comments as done.Oct 5 2017, 1:58 PM
rnk updated this revision to Diff 117886.Oct 5 2017, 1:58 PM
  • comment
dschuff accepted this revision.Oct 5 2017, 2:02 PM
thegameg accepted this revision.Oct 5 2017, 2:17 PM
rnk added a comment.Oct 6 2017, 11:18 AM

I know I've only waited a day for @qcolombet and that's not much time, but I'm going to land this because the requirements checking change actually doesn't change our current functionality. Before this change, UsesCalleeSaves is initialized to false, so PEI doesn't require NoVRegs. This will actually give us better runtime checking.

This revision was automatically updated to reflect the committed changes.
qcolombet edited edge metadata.Oct 6 2017, 1:40 PM

Sorry Reid @rnk, I didn't know you were waiting for me. I saw already two people were saying it's good and I thought we didn't need a third one :).

It looks good to me too!