[PATCH] D129506: Move instruction predicate verification to emitInstruction

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:175-176
+  // FIXME: Enable feature predicate checks once all the test pass.
+  // AMDGPU_MC::verifyInstructionPredicates(MI->getOpcode(),
+  //                                        getSubtargetInfo().getFeatureBits());
+
----------------
arsenm wrote:
> Why put this in the asm printer? Why not put this in the machine verifier?
The benefits of having the method in AMDGPU_MC is that it can be moved anywhere the target likes.

Unfortunately adding it to the machine verifier does mean that there are a new class of errors that were not seen before - from mir tests with incorrect target features for the used instructions. I wanted to make this change without loosing any of the test coverage that we already have, and don't have a huge amount of bandwidth to sort through all those failing mir tests.

I was contemplating whether to call verifyInstructionPredicates from verifyInstruction on AArch64, but it didn't catch many actual errors other than benign issues in mir tests so I ended up undoing it. Feel free to adjust as you see is best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129506