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

[Ulrich Weigand via llvm-commits]

uweigand added a comment.

In https://reviews.llvm.org/D21809#504834, @zhanjunl wrote:

> In https://reviews.llvm.org/D21809#502103, @uweigand wrote:
>
> > 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.


Well, the mnemonic would be just ".insn", and "e" would be the first operand, decoded as fixed string.  If you simply use ".insn e, ..." as an AsmParser pattern, you should see something like this in the generated SystemZGenAsmMatcher.inc file:

static const MatchEntry MatchTable0[] = {

  { 0 /* .insn */, SystemZ::AsmInsnE, Convert__U16Imm1_1, 0, { MCK_e, MCK_U16Imm }, },
  { 0 /* .insn */, SystemZ::AsmInsnRI, Convert__U32Imm1_1__AnyGRFP1_2__S16Imm1_3, 0, { MCK_ri, MCK_U32Imm, MCK_AnyGRFP, MCK_S16Imm }, },
  { 0 /* .insn */, SystemZ::AsmInsnRIE, Convert__U48Imm1_1__AnyGRFP1_2__AnyGRFP1_3__PCRel161_4, 0, { MCK_rie, MCK_U48Imm, MCK_AnyGRFP, MCK_AnyGRFP, MCK_PCRel16 }, },

where the MCK_e, MCK_ri etc are matched as string constants via matchTokenString.

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


Hmm.  Maybe just use DirectiveInsnRSY then, since RSE is just the same as RSY except that only 12-bit displacements are used (which the insn pattern already enforces via addr12only).


https://reviews.llvm.org/D21809