[PATCH] D80538: [MachineVerifier] Add a new RewriteTied flag to fix verify two-address constraint error

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:158
     Selected,
-    LastProperty = Selected,
+    RewriteTied,
+    LastProperty = RewriteTied,
----------------
I think a more descriptive name would be something like `TiedOpsRewritten`.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1659
     unsigned DefIdx;
-    if (!MRI->isSSA() && MO->isUse() &&
-        MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
+    if (MF->getProperties().hasProperty(
+            MachineFunctionProperties::Property::RewriteTied) &&
----------------
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.


================
Comment at: llvm/test/CodeGen/PowerPC/two-address-crash.mir:1
-# RUN: not --crash llc -mtriple=ppc32-- %s -run-pass=phi-node-elimination \
-# RUN:   -verify-machineinstrs -o /dev/null 2>&1 | FileCheck %s
+# RUN: llc -mtriple=ppc32-- %s -run-pass=phi-node-elimination \
+# RUN:   -verify-machineinstrs -o /dev/null 2>&1
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80538/new/

https://reviews.llvm.org/D80538