[PATCH] D56682: [GlobalISel][AArch64] Add isel support for FP16 vector @llvm.ceil

[Amara Emerson via Phabricator via llvm-commits]

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

LGTM with some changes.



================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:523
+            getMinClassForRegBank(DstRegBank, SrcSize, true);
+
+        // Get the appropriate subregister for the destination.
----------------
We should have an assert here for SubregRC != null..


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1789
+      AArch64::FPRRegBankID) {
+    LLVM_DEBUG(dbgs() << "Vector-to-GPR unmerges not supported yet.\n");
+    return false;
----------------
This debug comment's not quite accurate, as G_UNMERGE can also be used to split a large scalar into smaller scalar components.  I also think we should check that the source operand is also on the FPR register bank to avoid accidentally trying to handle weird fpr = unmerge(gpr) cases.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:1836
+  // Stores the registers we'll be copying from.
+  std::vector<unsigned> InsertRegs;
+
----------------
Use SmallVector to avoid heap allocs?


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:674
+  case TargetOpcode::G_BUILD_VECTOR:
+    // If the first operand belongs to a FPR register bank, then make sure
+    // that we preserve that.
----------------
clarify s/first operand/first source operand


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:682
+
+    // Get the instruction that defined the BUILD_VECTOR, and check if it's
+    // a floating point operation.
----------------
clarify s/defined the BUILD_VECTOR/defined the source operand reg


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

https://reviews.llvm.org/D56682