[PATCH] D22008: GlobalISel: implement low-level type suitable for MachineInstr selection

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Tim,

Looks pretty good to me.
I'd like to keep (at least for now), the type recording capabilities in register bank info though. I believe making a second constructor for LLT that takes SVT is going to be useful anyway.

Cheers,
-Quentin


================
Comment at: lib/CodeGen/GlobalISel/RegisterBankInfo.cpp:107
@@ -106,3 +100,1 @@
-        recordRegBankForType(getRegBank(ID), SVT);
-
     // Walk through all sub register classes and push them into the worklist.
----------------
We should be able to keep this functionality by providing a conversion from SVT to LLT, like we do for Type, right?

================
Comment at: lib/CodeGen/LowLevelType.cpp:24
@@ +23,3 @@
+    ScalarSize = VTy->getElementType()->getPrimitiveSizeInBits();
+    NumElements = VTy->getNumElements();
+  } else if (Ty->isSized()) {
----------------
Don't you say we should allow <1 x Ty> kind of vector?
I do not see that here.

================
Comment at: lib/CodeGen/LowLevelType.cpp:30
@@ +29,3 @@
+    ScalarSize = Ty->getPrimitiveSizeInBits();
+    NumElements = 1;
+  } else {
----------------
One thing to keep in mind is that, at some point, when we will want to do optimizations, we should have a way to know that the padding bits are garbage.
I do not expect this to be handle here, but I should think a way of rebuilding that information like computeKnownBits.

That will probably affect the legalization (expansion) of loads and stores when optimizations are set.

================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:193
@@ +192,2 @@
+  return Mapping;
+}
----------------
I guess we do not need that if we keep the types recording in RegisterBankInfo. 

================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir:72
@@ -73,1 +71,3 @@
+    ; CHECK:      %0(32) = G_ADD s32 %w0
+    %0(32) = G_ADD s32 %w0, %w0
 ...
----------------
Thanks for fixing the typo as well, I meant to do it at some point :P.


Repository:
  rL LLVM

http://reviews.llvm.org/D22008