This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Allow logical immediates to have all-1 in top bits
ClosedPublic

Authored by MaskRay on Mar 9 2020, 5:52 PM.

Details

Summary

So that constant expressions like the following are permitted:

and w0, w0, #~(0xfe<<24)
and w1, w1, #~(0xff<<24)

The behavior matches GNU as (opcodes/aarch64-opc.c:aarch64_logical_immediate_p).

Diff Detail

Event Timeline

MaskRay created this revision.Mar 9 2020, 5:52 PM
MaskRay updated this revision to Diff 249259.Mar 9 2020, 5:57 PM

Add a comment.

It seems a bit odd to add assembly support for something which we also can't disassemble? It seems like a basic self-test that we should have.

It seems a bit odd to add assembly support for something which we also can't disassemble? It seems like a basic self-test that we should have.

Not odd. We can't assemble

and w0, w0, #~(0xfe<<24)
and w1, w1, #~(0xff<<24)

This patch teaches MC to assemble them. It is just that the disassembly is not a round trip.

For dupm z0.h, #0x7f00, it is indeed odd. GNU as behaves the same, though. I know too little about SVE to fix it properly by myself. I hope more experienced contributors can do that as a follow-up.

Friendly ping

I think you should try to figure what the issue is with SVE... It's not good practise to scratch your particular itch and then not fix the bug it exposes too.

MaskRay added a comment.EditedMar 17 2020, 9:19 AM

I think you should try to figure what the issue is with SVE... It's not good practise to scratch your particular itch and then not fix the bug it exposes too.

May I say that this suggests a quality of implementation issue of the SVE patches themselves? GNU as has the same problem... The organization of code makes is sorta clear that if we intend to fix SVE, it will be a separate patch.

MaskRay added a comment.EditedMar 26 2020, 9:25 AM

Ping:)

As I said, I think the SVE problem is very likely its own QoI problem (GNU as has the same issue). I hope an experienced contributor can fix that in a subsequent change, but it should not block this change.

sdesmalen added inline comments.Apr 2 2020, 4:06 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
761

Is this the same as:

uint64_t Upper = UINT64_C(-1) << (sizeof(T) * 8);

?

llvm/test/MC/AArch64/SVE/mov.s
210

Disassembling to dupm z0.h, #0x7f00 makes sense here.

The mov alias is preferred when SVEMoveMaskPreferred is true. This instructino would splat 0x7f00 into each 16-bit lane of z0, resulting in 0x7f007f007f007f00. For this input, SVEMoveMaskPreferred returns false, so dupm is the correct disassembly.

So I'd say the fixme can be removed.

MaskRay updated this revision to Diff 254538.Apr 2 2020, 8:57 AM
MaskRay marked 4 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Delete FIXME. According to sdesmalen, "Disassembling to dupm z0.h, #0x7f00 makes sense here."

MaskRay added inline comments.Apr 2 2020, 8:59 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
761

http://eel.is/c++draft//expr.shift

The behavior is undefined if the right operand is negative, or greater than or equal to the width of the promoted left operand.

This avoids direct left shift by 64.

This revision is now accepted and ready to land.Apr 6 2020, 3:24 AM

Thanks for the review!

This revision was automatically updated to reflect the committed changes.