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

[Zhan Jun Liau via llvm-commits]

zhanjunl added a comment.

In https://reviews.llvm.org/D21809#502103, @uweigand wrote:

> 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'll split off the changes into a different diff.

> 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.


I would have liked to use ".insn e" for the mnemonic as well, but from my understanding, TableGen removes all white spaces after parsing the string for tokens, so I couldn't find a way to get TableGen to generate ".insn e" as the asm mnemonic. Which is why I created two duplicated instructions. I'll try to do a bit more digging to see if there's another way around it.

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


I wasn't sure whether vector registers should be recognized since there are no .insn directives for vector instructions, but I guess for GCC compatibility it makes sense. I'll fix this up.

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


I'm not sure we want to have a newly created InsnRSE, since RSE isn't a valid instruction format and I don't want to confuse any future users into thinking it's a valid format to use.


https://reviews.llvm.org/D21809