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

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.
Herald added a subscriber: dmgreen.

@dsanders I agree that this patch addresses a real problem, and that there is currently a generalized lack of target-specific checks. AMDGPU seems to be the target with the most comprehensive set of checks (although I think it's mostly in the S.I. subtarget). Inspired by this patch we also submitted a patch to verify the immediates of `MachineInstr`s for the RISC-V target (D67397 <https://reviews.llvm.org/D67397>). The fact that invalid immediate operands just get silently processed (emitted in asm form, truncated in obj form) in nearly all targets is a good indicator that there is space for improvements, and that unrelated changes at any point might reveal latent bugs. Hopefully having more of this infrastructure in place will further encourage targets to add more checks. Still, I'm not sure adding a pass is the best way to accomplish this.

Why have the distinction between `MachineVerifier` and `TargetMachineVerifier`? Presumably you are going to update this patch to remove the `TargetMachineVerifier::verifyMI` method, since that functionality is already covered the by `TargetInstrInfo::verifyInstruction` hook. Is there any fundamental reason why the `MF`/`MBB` checks should be part of this new pass while the `MI` check is a hook? I think not. I think all three verifications should be hooks, appropriately called by the `MachineVerifier`. Having the MachineVerifier in charge of the high-level logic and delegating to the target to verify target-specific information is consistent with LLVM practice. Now, I'm assuming we can do the appropriate target-independent verifications in MachineVerifier before calling the hooks, which I agree nicely simplifies the target-specific checks, but I may be wrong about that. Assuming we can, there's IMO not really a good motivation to split the verifications in two passes. I imagine a single pass will be more cache friendly too. One possible issue is that it might not be clear where to put the `MF`/`MBB` hooks, since it seems none of the existing target interfaces currently are perfect matches. It might be worth considering adding a new target interface (or refactoring existing ones?). BTW, if you do that, consider if the `TargetMachineVerifier::verifyMI` hook should be moved there, or if it makes sense to remain part of the `TargetMachineVerifier`. Answering this question of where to put the hooks might also clarify what is the best way to verify machine operands. For my D67397 <https://reviews.llvm.org/D67397> patch I did the `MachineInstr` verification based on the `MCOperandPredicates`, and I had to duplicate some of the logic for that, since the MC layer and the `MachineInstrs` have a different notion of immediates. Having the somewhat higher-level `MachineInstr` be checked based on the lower-level MC info is also not great, although not unprecedented. It might make sense to have a more neutral place as a ground truth that could be used to check properties common to `MachineInstr`s and `MCInstr`s, and that place might be where you would put those two additional hooks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64650