Page MenuHomePhabricator

[ARM] Fix test inlineasm-X-allocation.ll
ClosedPublic

Authored by SjoerdMeijer on Oct 26 2018, 2:07 AM.

Details

Summary

While working on FileCheck producing better diagnostics in D53710,
I noticed that this test case is broken in a few different ways, and
wanted to check the changes I made (because there are few things I
don't understand here).

The problem was that labels vfp-CHECK and novfp-CHECK where not
checked, so the test wasn't doing anything. Replacing this with just vfp
and novfp revealed a test failure in the novfp case: it was actually
producing vadd.f32 s0, s0, s0 and not using the GPR registers. But I
don't see how we can ever get GPR registers here, thus I don't think
this test makes much sense, and I have just removed it.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Oct 26 2018, 2:07 AM

IIRC the contents of the asm string shouldn't matter here, and we would expect to get an error in the assembler instead when trying to assemble the output (which is fine, since this is a misuse of the X constraint).

However, it seems wrong for the compiler to assign s0 as an operand without having FP registers available.

However, it seems wrong for the compiler to assign s0 as an operand without having FP registers available.

Okay, to be more precise, I don't understand what exactly we are expecting here. Why are we expecting vadd.f32 to work on integer registers? What I mean is that I see -neon is specified, but why in the absense of that do we expect integer registers here. That doesn't make much sense for vadd.f32, isn't it?

The initial intent was to make sure that we are forcing the operands to not be VFP registers, so I would have expected to get a vadd.f32 r0, r0, r0. However, since the instruction is invalid it doesn't make a good test (as you observed).

Using a

call void asm sideeffect "// foo $0, $0, $0", "X" (float %f) nounwind

instruction would work just as well for this test and it wouldn't produce an assembler error.

Ah, I finally got it, and many thanks for explaining! I know see too that the mnemonic is totally irrelevant here (I got hugely distracted by vadd.f32).
I've learned something today! :-)

I think I've now tidied up the test case, and it is a bit more sensible.

This time with the correct diff attached; the previous one had the test cases swapped.

sbaranga accepted this revision.Oct 26 2018, 7:52 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Oct 26 2018, 7:52 AM
This revision was automatically updated to reflect the committed changes.