[PATCH] D16649: [AArch64] Custom lower some extloads

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Chad,

A couple of comments.

Thanks,

James


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2334
@@ +2333,3 @@
+  MVT SclrLoadTy = MVT::i8;
+  for (MVT VT : MVT::integer_vector_valuetypes())
+    if (TLI.isTypeLegal(VT))
----------------
Is this guaranteed to iterate in size order? Perhaps we sholud add a comment indicating that we rely upon this? (and an assert that the size of every item is greater than that of the preceding item?)

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2343
@@ +2342,3 @@
+
+  assert((Ext != ISD::SEXTLOAD || NumLoads == 1) &&
+         "Can only lower sext loads with a single scalar load!");
----------------
I'm worried as to how this assert can fire. The logic for getting here looks the same for extload and sextload, so how is this OK for extloads and not for sextloads (and where do sextloads that would fire this assert get filtered out?)

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2390
@@ +2389,3 @@
+  // Sign extend the vector. This will be legalized to a shuffle and shifts.
+  if (Ext == ISD::SEXTLOAD) {
+    SDValue Shuff = DAG.getSignExtendVectorInReg(SlicedVec, dl, RegVT);
----------------
Why can't we zextload here (as well as sext and extloading)? Is there an instruction missing in the ISA for zexting?


http://reviews.llvm.org/D16649