[PATCH] D63938: [ARM] Stop using scalar FP instructions in integer-only MVE mode.

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

LGTM with one nit.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:599
+  if (!Subtarget->useSoftFloat() && !Subtarget->isThumb1Only()) {
+    if (Subtarget->hasFPRegs()) {
+      addRegisterClass(MVT::f32, &ARM::SPRRegClass);
----------------
This can still be the same if? To drop a level of indentation.


================
Comment at: llvm/test/CodeGen/Thumb2/float-ops.ll:268
+; ONLYREGS: lsls    r2, r2, #31
+; ONLYREGS: vmovne.f32      s2, s0
 ; HARD: lsls    r0, r0, #31
----------------
simon_tatham wrote:
> dmgreen wrote:
> > This is worse than before by the looks of it? We move things into fp registers just to move them out again.
> Perhaps that's true, but I'd rather fix the correctness first and get a set of regression tests passing, and //then// we can worry about recovering the performance with those tests in place to prevent breaking correctness again.
> 
> Especially since in the general case it's not really clear how you //should// choose which kind of register to use for a value. Keeping it in an FP register is obviously wasteful in this case, but in another case where register pressure is high, might it save a spill or two? I think getting the right answers in cases larger than this simple one might not be trivial.
My worry is that this will mean every floating point load becomes a vldr, which just ends up being moved into a gpr. This probably isn't a huge deal for performance, as you will likely always be calling a __aeabi_fadd type function, but the codesize would increase quite a bit. I couldn't think of a reason when you _would_ want to load using a vldr (at least it would be fairly uncommon). I imagine that almost every operation would actually be done on a gpr for floats.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63938/new/

https://reviews.llvm.org/D63938