[PATCH] D58460: [AArch64] Optimize floating point materialization

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Not sure I like the duplicated logic in getExpandImmCost; it doesn't have good test coverage, and it could fall out of sync in the future.  That said, I've been considering refactoring the code in expandMOVImm anyway, to split the actual instruction emission away from the logic that figures out the appropriate sequence.  Basically, the idea would be that instead of returning a number from getExpandImmCost, you return an abstraction of the instruction sequence: an array that contains, for each instruction, the appropriate opcode and immediate.  isFPImmLegal just uses the number of elements in the array, while expandMOVImm actually emits instructions based on the array.  I think this would shrink the code overall because the logic for building instructions is currently duplicated multiple times.  (I was considering it more in the context of adding more possible sequences, but it works here as well.)



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:5411
+    // sequence vs. a two instruction constant-pool load.  So we limit to
+    // at maximum of 2 moves to match and adrl+ldr cost.
+    int NumInst = AArch64_AM::getExpandImmCost(ImmInt.getZExtValue(),
----------------
Is this comment redundant?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:5414
+                                               VT.getSizeInBits());
+    IsLegal = NumInst <= forCodeSize ? 1 : 2;
+  }
----------------
Where is forCodeSize defined?


================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h:1054
+  if (OneChunks >= (BitSize / 16) - 2 || ZeroChunks >= (BitSize / 16) - 2)
+    return 2;
+
----------------
"return 2" isn't quite right; it could be 1.


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

https://reviews.llvm.org/D58460