This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Make sure assembler rejects PC as an operand for VMOV.F16
Needs ReviewPublic

Authored by avieira on Sep 22 2017, 5:46 AM.

Details

Summary

Hi,

This restricts the general purpose register of vmov.f16 such that the assembler does not accept PC as a valid operand.

Is this OK?

Cheers,
Andre

Diff Detail

Event Timeline

avieira created this revision.Sep 22 2017, 5:46 AM
rengolin edited edge metadata.Sep 22 2017, 8:08 AM

All similar VMOVs seem to have the same restriction, maybe we should update the whole lot at the same time?

test/MC/ARM/fullfp16.s
261

shouldn't this be s0 instead of r0?

All similar VMOVs seem to have the same restriction, maybe we should update the whole lot at the same time?

Good point, suffering from a bit of tunnel vision there. I will go through the rest of the VMOV's and reupload a new diff.

As for the testcase, it should indeed be s0 on both.

I took the opportunity to also look at the SP operand. Currently it is not rejected either, and it should for non Armv8-A architectures running in Thumb mode.

Now I am looking at rGPR for this, but GPR rejects SP for non Armv8-A Arm mode too, and that is not what we want here. I am now investigating further, to see whether it would make sense to actually change the behavior of rGPR to reflect the behavior we need and I found an instruction that is using rGPR but I dont think it should. The VLD1LNd8_UPD instruction, has the third Rm operand down as a rGPR, but that means it would accept SP for Armv8-A architectures and that conflicts with what I am seeing in the Armv8-A ARM ARM. The only variants of VLD{1,2,4,5} that use Rm as an operand in the instruction do not accept Rm to be either SP or PC, these are encodings reserved for the offset and post-indexed variants. So I'm guessing this breaks the current use of rGPR already.

All in all, this looks like a bigger job than I initially hoped for. So it might take a little longer to investigate and produce a patch...

All in all, this looks like a bigger job than I initially hoped for. So it might take a little longer to investigate and produce a patch...

It should be simple to do this into multiple smaller jobs, starting with the one that breaks for you, and then continuing for the rest of the affected classes.

Hi,

I started working on the first set of changes for this, taking the vmov instructions only. I added a new register class rGPR2 which behaves the way we want for these instructions. However, I ran into a failure in the CodeGen/ARM/fast-isel-align.ll test when it tried to generate the following:
%vreg6<def> = VMOVRS %vreg10, pred:14, pred:%noreg; GPR:%vreg6 SPR:%vreg10

This test has the flag -verify-machineinstrs and this check breaks saying it expected rGPR2 and not GPR. I also tried to use the existing GPRnopc instead, to make sure I wasn't just missing something somewhere, but I get the same error when verifying the machine instructions. The check is done based on the SuperClass, basically it checks that RC->hasSuperClassEq(DRC) and otherwise it errors out, where RC here is the GPRRegClass and DRC is either rGPR2RegClass (first attempt) or GPRnopcRegClass (second attempt).

After some playing around I found that adding the rGPR2RegClass as a register class in ARMISelLowering solved the issue with VMRS. However, I got a issue with VMOVSR, when the input register that is being used is created as a rGPR or rGPRnopc.

The error reads something like:

    • Bad machine code: Illegal virtual register for instruction ***
  • function: unaligned_f32_load
  • basic block: BB#0 entry (0x8313e98)
  • instruction: %vreg9<def> = VMOVSR
  • operand 1: %vreg8

Expected a rGPR2 register, but got a GPRnopc register

The input register in question here is vreg8 from:
%vreg8<def> = LDRi12 %vreg10, 2, pred:14, pred:%noreg; GPRnopc:%vreg8 rGPR:%vreg10
%vreg9<def> = VMOVSR %vreg8, pred:14, pred:%noreg; SPR:%vreg9 GPRnopc:%vreg8

So I still have problems of register classes not being fully compatible. I was wondering whether anyone has any insight on how to tackle this or where I could look for more information on how all this fits together?

Thanks!

Hi Andre,

I've not looked at fast isel much before, but it looks like it is responsible for creating virtual registers of the correct register class for the instructions it's creating. If you grep for createResultReg in ARMFastISel.cpp, there are a few places where it has to select between GPRRegClass and rGPRRegClass, have you tried updating that code to use rGPR2RegClass when building a VMOVSR?

Just a nit, why did you call it rGPR2? If this is a noPC/noSP class, than a longer but more meaningful name like rGPRnopcsp would be preferable.

On the invalid register class errors, you may find that the isel/regalloc are annealed with the current register descriptions, so it may take a bit of a beating.

Don't forget to also look at Global ISel.

Just a nit, why did you call it rGPR2? If this is a noPC/noSP class, than a longer but more meaningful name like rGPRnopcsp would be preferable.

Sure, I just picked rGPR2 as a placeholder, I expect in the end of this to replace almost all rGPR's with this new one and then revisit the names. But rGPRnopcsp is just as good a placeholder, if not better.

On the invalid register class errors, you may find that the isel/regalloc are annealed with the current register descriptions, so it may take a bit of a beating.

Don't forget to also look at Global ISel.

Given Oliver's comments I was able to find the location where that LDR-VMOV was being created in FastISel and changed it there. This fixed this particular issue, but I have also found other codegen changes due to register allocation that I need to go look at... even with instructions/code that uses no vmov's.

Thanks for the tips!