[PATCH] D21809: [SystemZ] Add support for the .insn directive.

[Ulrich Weigand via llvm-commits]

uweigand added a comment.

Thanks for the rework, this is looking much better now!

A couple of additional comments / requests:

Could you please split off adding those other insns like PR, MVCK, ... into a separate patch?  This is really an independent change that can be reviewed separately and committed ahead of the .insn patch.

I don't really like the code duplication between the Insn / AsmInsn patterns.  I can see why we need a isCodeGenOnly as well as a isAsmParserOnly pattern (so that the disassembler doesn't get confused), but those should then get generated from a multiclass template.  See e.g. how this is done with the CompareBranches multiclass.

In addition, it doesn't seem a good idea to have the AsmInsn patterns recognize "instructions" like InsnE.  This means that assembly files that contain the literal string InsnE will get recognized here, which is not really what we want.  I'm wondering if we cannot simply also use ".insn e,..." as the string for the AsmInsn patterns as well -- if we do that, I think the auto-generated matchers should use literal string matching to detect the correct pattern to use, which means that the extra table getOpcodeFromFormat would no longer be necessary -- another duplication eliminated.

Your routine parseAnyGRFPRegister accepts integers, %rX and %fX.  GAS in addition accepts %vX (for X < 16) as well.

For consistency, I think DirectiveInsnRSE should derive from (a newly created) InsnRSE, just like all the other DirectiveInsn patterns.


https://reviews.llvm.org/D21809