Page MenuHomePhabricator

[MachineVerifier] Add a new TiedOpsRewritten flag to fix verify two-address constraint error
ClosedPublic

Authored by ZhangKang on May 26 2020, 1:03 AM.

Details

Summary

Currently, MachineVerifier will attempt to verify that tied operands satisfy register constraints
as soon as the function is no longer in SSA form. However, PHIElimination will take the
function out of SSA form while TwoAddressInstructionPass will actually rewrite tied operands
to match the constraints. PHIElimination runs first in the pipeline.
Therefore, whenever the MachineVerifier is run after PHIElimination, it will encounter
verification errors on any tied operands.

This patch adds a function property called TiedOpsRewritten that will be set by
TwoAddressInstructionPass and will control when the verifier checks tied operands.

This patch can fix 4752 cases error for two-address constraint verification.

Diff Detail

Event Timeline

ZhangKang created this revision.May 26 2020, 1:03 AM

The TwoAddressInstructionPass will rewrite the tied to meet the RegConstraint.
For example:
The TwoAddressInstructionPass will rewrite below IR

%10:gprc = RLWIMI killed %9:gprc(tied-def 0), killed %0:gprc, 1, 0, 30

to:

%10:gprc = RLWIMI killed %10:gprc(tied-def 0), killed %0:gprc, 1, 0, 30
ZhangKang edited the summary of this revision. (Show Details)May 26 2020, 1:05 AM
nemanjai requested changes to this revision.May 26 2020, 4:15 AM

Just to be clear, I don't actually have an issue with the code changes. The reason I am requesting changes is to ensure that the description (and presumably commit message) adequately describes the problem being solved.

llvm/include/llvm/CodeGen/MachineFunction.h
158–159

I think a more descriptive name would be something like TiedOpsRewritten.

llvm/lib/CodeGen/MachineVerifier.cpp
1644–1646

The description does not adequately describe why this is needed. Namely, we will set this property at the same point in the code as the call to MRI->leaveSSA(). So how do the following two checks differ:

MF->getProperties().hasProperty(
    MachineFunctionProperties::Property::RewriteTied)

vs.
!MRI->isSSA()

It would appear that the issue is when we run the verifier after PHI elimination and not after Two Address.

llvm/test/CodeGen/PowerPC/two-address-crash.mir
1–4

I think it would be more useful to -start-before=phi-node-elimination and thereby test the verification after all the passes so that verification of the tied operands is actually performed.

This revision now requires changes to proceed.May 26 2020, 4:15 AM
ZhangKang marked 5 inline comments as done.May 26 2020, 7:47 PM
ZhangKang added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
158–159

Done.

llvm/lib/CodeGen/MachineVerifier.cpp
1644–1646

Have modified the comment.

llvm/test/CodeGen/PowerPC/two-address-crash.mir
1–4

In fact, if I use -start-before=phi-node-elimination, whether using my patch, there is no error, because the verification for phi-node-elimination pass and twoaddressinstruction pass have been disabled, that why we haven't gotten any error before.
But as you said, I still add -start-before=phi-node-elimination, because it can test the passes which have enabled verification.

ZhangKang updated this revision to Diff 266398.May 26 2020, 7:48 PM
ZhangKang marked 2 inline comments as done.

Modify the comments and test case.

ZhangKang removed reviewers: Restricted Project, nemanjai.May 26 2020, 7:50 PM
ZhangKang added reviewers: nemanjai, Restricted Project.
craig.topper added inline comments.May 26 2020, 8:39 PM
llvm/include/llvm/CodeGen/MachineFunction.h
147

Update comment to match the new enum name.

Update the comments.

ZhangKang marked 2 inline comments as done.May 26 2020, 10:21 PM
ZhangKang added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
147

Done.

lkail added a subscriber: lkail.May 26 2020, 10:28 PM

The title should be also rephrased.

ZhangKang retitled this revision from [MachineVerifier] Add a new RewriteTied flag to fix verify two-address constraint error to [MachineVerifier] Add a new TiedOpsRewritten flag to fix verify two-address constraint error.May 26 2020, 11:08 PM
ZhangKang edited the summary of this revision. (Show Details)
nemanjai accepted this revision.Jun 2 2020, 5:56 AM

The following sentence in the description is still incorrect:

We should verify two-address constraints after TwoAddressInstructionPass pass has been run, But in the MachineVerifier.cpp, we will
verify two-address constraints when the machine function is SSA, this is not correct.

I think it suffices for the description to just succinctly describe the problem. Something like:

Currently MachineVerifier will attempt to verify that tied operands satisfy register constraints
as soon as the function is no longer in SSA form. However, PHIElimination will take the
function out of SSA form while TwoAddressInstructionPass will actually rewrite tied operands
to match the constraints. PHIElimination runs first in the pipeline.
Therefore whenever the MachineVerifier is run after PHIElimination, it will encounter
verification errors on any tied operands.

This patch adds a function property called TiedOpsRewritten that will be set by 
TwoAddressInstructionPass and will control when the verifier checks tied operands.

Other than that, LGTM.

This revision is now accepted and ready to land.Jun 2 2020, 5:56 AM
ZhangKang edited the summary of this revision. (Show Details)Jun 2 2020, 7:52 AM
This revision was automatically updated to reflect the committed changes.
ZhangKang marked an inline comment as done.