[PATCH] D64650: Add a TargetMachineVerifier that runs along with the existing MachineVerifier

Fri Jul 12 13:41:53 PDT 2019

dsanders marked an inline comment as done.
dsanders added a comment.

In D64650#1583208 <https://reviews.llvm.org/D64650#1583208>, @arsenm wrote:

> The current target verification problems I have are:
>
> - Verifying legal register bank mappings, so I don't have to defend against them in InstructionSelect
> - Verifying G_INTRINSIC operand types and numbers. There's missing API to do this.
> - Verifying some target IR constraints, in an IR verifier
> - Verifying some special block level properties for exec masking, which is the only category I would probably want to put in this pass

Yep, the first two sound like a job for verifyInstruction() and the third sounds like a one-shot pass before IRTranslator/SelectionDAG. The fourth does sound like it makes sense for this pass.

What's missing for the G_INTRINSIC one?

================
Comment at: lib/Target/AArch64/AArch64MachineVerifier.cpp:79-92
+void AArch64MachineVerifier::verifyMI(const MachineFunction &MF,
+                                      const MachineBasicBlock &MBB,
+                                      const MachineInstr &MI,
+                                      const char *CallBanner) {
+  switch (MI.getOpcode()) {
+  case AArch64::SBFMWri:
+  case AArch64::UBFMWri:
----------------
arsenm wrote:
> While I do think there is a need for a target verifier pass, you don't need it to verify instruction level things like this. You can already do that in TargetInstrInfo::verifyInstruction
I didn't know about that function. That will work nicely for the trivial checks on my list. Thanks.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64650