[PATCH] [AArch64] Add logical pseudo instructions to MC AsmParser

Tim Northover t.p.northover at gmail.com
Thu Jul 10 07:02:59 PDT 2014

Hi Arnaud,

> For example, with this patch, the closest match for the assembler is the pseudo
> in one of the basic-a64-diagnostics tests, which leads to a weird diagnostic. Would
> there be a non convoluted way to disambiguate this, or give a lower priority to
> pseudos ?

Not that I'm aware of. The diagnostic doesn't look too bad to me.
There are certainly worse around in LLVM's assemblers.

I don't think "pseudo-instruction" or related terms is right here
though. LLVM actually has pseudo-instructions of various kinds, and
they're not used in this patch (fortunately -- that would have been
the wrong way to do it and I was a little worried before I opened up
the attachment).

+def logical_imm32_not_XFORM : SDNodeXForm<imm, [{

SDNodeXForms only apply to CodeGen, but you're not actually changing
any patterns so I think these are unnecessary. Similarly for the
PatLeaf instantiations. Operands can be much more minimal if they're
only used in InstAliases.

+  let PrintMethod = "printLogicalImm32";

I hope these are unnecessary too. Or do they get referred to in
generated files even if they're not called?

+                      string pseudo> {

Following my first comments, I think this is the wrong name. Perhaps
NegAlias or something? Even just "Alias" would be more accurate.

+  let AddedComplexity = 6 in

With no new CodeGen patterns, this seems unnecessary. Also untested,
since there aren't any .ll tests.

+    const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
+    assert(MCE && "Invalid logical immediate operand!");

If you use "cast" instead of "dyn_cast" it includes an assert.

Other than that, it looks reasonable to me.



More information about the llvm-commits mailing list