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

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang marked 5 inline comments as done.
ZhangKang added inline comments.


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


================
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) &&
----------------
nemanjai wrote:
> 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.
Have modified the comment.


================
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
----------------
nemanjai wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80538