This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Asm: Fix parsing of register aliases that have a name starting with 'z'
ClosedPublic

Authored by sdesmalen on Dec 19 2017, 2:38 AM.

Diff Detail

Event Timeline

sdesmalen created this revision.Dec 19 2017, 2:38 AM
fhahn added inline comments.Dec 19 2017, 4:46 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2010

I don't think we need this early exit here. If it's a vector register, matchRegisterNameAlias should return 0 and we return -1 later. We do not bail out for NEON early either.

fhahn added inline comments.Dec 19 2017, 4:50 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2010

Ok, I was wrong about matchRegisterNameAlias. But it seems the early exit is not needed, the MC test pass for me without it and we do not have a similar case for NEON.

sdesmalen added inline comments.Dec 19 2017, 5:57 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2010

This was added to enable the other test, where the alias is a vector register (that needs custom parsing), e.g.:

peter .req z6
add peter.s, z0.s, z0.s

However, in that case it seems wrong for 'matchRegisterNameAlias(..., RegKind::Scalar) to return the register number of an SVE vector. This is caused by MatchRegisterName() being generic to work for non-scalar registers as well, so I'll fix this.

sdesmalen updated this revision to Diff 127513.Dec 19 2017, 5:59 AM

Fixed issue where matchRegisterNameAlias(..., RegKind::Scalar) also matched non-scalar identifiers.

rnk accepted this revision.Dec 19 2017, 10:30 AM

Thanks!

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2005–2012

This is unused now.

This revision is now accepted and ready to land.Dec 19 2017, 10:30 AM

LGTM. The problem this solves is that MatchRegisterName parses GPRs, NEON and SVE registers, which leads to surprising behaviour, i.e. matchRegisterNameAlias with RegKind == Scalar would return > 0 for any (valid) Neon & SVE register as well. From the comments and the behaviour, it seems like tryParseRegister should only parse scalar registers, which this patch implements. I think it's worth adding a comment making that clear.

Adding Eric, in case he has some more thoughts!

fhahn accepted this revision.Dec 19 2017, 1:53 PM
fhahn added a comment.Dec 19 2017, 2:06 PM

One more thing for completeness (there's no need to address this in this patch): currently we try to parse the register before looking up aliases so the following will fail

	z1 .req x6
	add z1, x0, x0
	.unreq z1
// CHECK: add x6, x0, x0

I think GAS tries to apply aliases first, with the following reasoning: Before the SVE registers were added, the snippet above was valid. Now it is not valid any more, as it will parse z1 as a SVE register (the same problem also exists with NEON vector registers)

sdesmalen closed this revision.Dec 20 2017, 1:46 AM

@rnk Thanks for providing the reduced test-case!
@fhahn Interesting, it seems the assembler had this behaviour even before the SVE patches changed the matchRegisterNameAlias() method.