This is an archive of the discontinued LLVM Phabricator instance.

Move instruction predicate verification to emitInstruction
ClosedPublic

Authored by dmgreen on Jul 11 2022, 11:42 AM.

Details

Summary

D25618 added a method to verify the instruction predicates for an emitted instruction, through verifyInstructionPredicates added into <Target>MCCodeEmitter::encodeInstruction. This is a very useful idea, but the implementation inside MCCodeEmitter made it only fire for object files, not assembly which most of the llvm test suite uses.

This patch moves the code into the <Target>_MC::verifyInstructionPredicates method, inside the InstrInfo. The allows it to be called from other places, such as in this patch where it is called from the <Target>AsmPrinter::emitInstruction methods which should trigger for both assembly and object files. It can also be called from other places such as verifyInstruction, but that is not done here (it tends to catch errors earlier, but in reality just shows all the mir tests that have incorrect feature predicates). The interface was also simplified slightly, moving computeAvailableFeatures into the function so that it does not need to be called externally.

The AArch64, ARM, AMDGPU (but not R600), AVR, Mips and X86 backends all currently show errors in the test-suite, so have been disabled with FIXME comments.

Diff Detail

Event Timeline

dmgreen created this revision.Jul 11 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 11:42 AM
dmgreen requested review of this revision.Jul 11 2022, 11:42 AM
foad accepted this revision.Jul 12 2022, 2:08 AM

Thank you for doing this! As mentioned on D25618 I have tried something similar in the past. AMDGPU and generic parts LGTM.

This revision is now accepted and ready to land.Jul 12 2022, 2:08 AM

Thanks! I think I've got the AArch64 tests all working too, so will adjust that before committing so long as it passes all the tests.

Arm will need more work before it can be enabled, and other targets I am less sure about. A lot of the errors are just benign failures from missing features, but at least some of the Arm failures look like real problems.

This revision was landed with ongoing or failed builds.Jul 13 2022, 4:53 AM
This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Jul 13 2022, 7:17 AM
llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
175–176

Why put this in the asm printer? Why not put this in the machine verifier?

dmgreen added inline comments.Jul 14 2022, 1:21 AM
llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
175–176

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.