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

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz marked 3 inline comments as done.
zatrazz added a comment.

In D58460#1411040 <https://reviews.llvm.org/D58460#1411040>, @efriedma wrote:

> 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.)


Right, I think I can follow this idea and rewrite my patch if you don't mind.



================
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(),
----------------
efriedma wrote:
> Is this comment redundant?
Indeed, I will remove it.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:5414
+                                               VT.getSizeInBits());
+    IsLegal = NumInst <= forCodeSize ? 1 : 2;
+  }
----------------
efriedma wrote:
> Where is forCodeSize defined?
It is an artifact from a wrong rebase, I will fix it (it is meant for a different patch).


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


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

https://reviews.llvm.org/D58460