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

[Arnaud Allard de Grandmaison]

On Thu, Jul 10, 2014 at 4:02 PM, Tim Northover <t.p.northover at gmail.com>
wrote:

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

Those codegen stuff definitely looked unnecessary to me, but I did not even
try without those :(
I have removed the PatLeaf and XForms as well.


> +  let PrintMethod = "printLogicalImm32";
>
> I hope these are unnecessary too. Or do they get referred to in
> generated files even if they're not called?
>

You're right. They are not necessary.


> +                      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's settle for Alias then.


>
> +  let AddedComplexity = 6 in
>
> With no new CodeGen patterns, this seems unnecessary. Also untested,
> since there aren't any .ll tests.
>
>
I had to move it from ouside of the multiclass, to where it is used inside
the multiclass, because InstAlias does not have those. As you point, the
alias have no codegen pattern.

I have not changed the functionality there, so no additionnal .ll test is
necessary -- I think.


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

Changed the patch to use cast instead.

By the way, this could applies in many places around. If you believe it is
worth, than I can fix those other places (and refactor a bit) in a separate
patch. This would make the code more readable.


> Other than that, it looks reasonable to me.
>

There was quite a lot of fixes :(

Thanks for the quick review !

I attached the updated patch. I left the fixme note in
basic-a64-diagnostics; I will remove it before committing if you think we
just don't care. I also changed pseudo to alias in the commit message or
comments.


> Cheers.
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140710/01c41f06/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AArch64-Add-logical-alias-instructions-to-MC-AsmPars.patch
Type: text/x-patch
Size: 10743 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140710/01c41f06/attachment.bin>