[PATCH] D19475: [mips] Clang generates unaligned offset for MSA instruction st.d

[Daniel Sanders via llvm-commits]

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

I think we're nearly there. My main concern is the fragility of the alignment check if we do it where it currently is.


================
Comment at: lib/Target/Mips/MipsISelDAGToDAG.h:79-92
@@ -78,4 +78,16 @@
 
-  /// Match addr+simm10 and addr
-  virtual bool selectIntAddrMSA(SDValue Addr, SDValue &Base,
-                                SDValue &Offset) const;
+  /// Match addr+simm10/11/12/13 and addr
+  virtual bool selectIntAddrMSAAlign(SDValue Addr, SDValue &Base,
+                                     SDValue &Offset, unsigned Align) const;
+
+  virtual bool selectIntAddrMSAAlign1(SDValue Addr, SDValue &Base,
+                                      SDValue &Offset) const;
+
+  virtual bool selectIntAddrMSAAlign2(SDValue Addr, SDValue &Base,
+                                      SDValue &Offset) const;
+
+  virtual bool selectIntAddrMSAAlign4(SDValue Addr, SDValue &Base,
+                                      SDValue &Offset) const;
+
+  virtual bool selectIntAddrMSAAlign8(SDValue Addr, SDValue &Base,
+                                      SDValue &Offset) const;
----------------
One last naming nit: We ought to switch out the mention of MSA for a mention of the immediate size and shift amount (e.g. selectAddrSimm10Lsl2) since these can be re-used for non-MSA purposes too. Removing the reference to 'MSA' should prevent people thinking it has MSA-specific special cases in the future.

================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:472-492
@@ +471,23 @@
+                                               SDValue &Offset,
+                                               unsigned Align) const {
+  // Calculate number of bits for simm10/simm11/simm12/simm13 depending on
+  // the alignment.
+  unsigned Bits;
+  switch (Align) {
+  case 1:
+    Bits = 10;
+    break;
+  case 2:
+    Bits = 11;
+    break;
+  case 4:
+    Bits = 12;
+    break;
+  case 8:
+    Bits = 13;
+    break;
+  default:
+    Bits = 10;
+    break;
+  }
+
----------------
If we have a shift amount rather than the alignment (like in the MC layer) then we can directly use something like '10 + ShiftAmount' rather than having to convert.

================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:478-482
@@ +477,7 @@
+  case 1:
+    Bits = 10;
+    break;
+  case 2:
+    Bits = 11;
+    break;
+  case 4:
----------------
Yes, you're right.

================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:497-505
@@ +496,11 @@
+    // eliminateFrameIndex.
+    if (!isa<FrameIndexSDNode>(Base)) {
+      if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Offset)) {
+        unsigned CnstOff = CN->getZExtValue();
+        unsigned OffsetDiff = OffsetToAlignment(CnstOff, Align);
+        // If offset is not aligned, call selectAddrDefault
+        if (OffsetDiff)
+          return selectAddrDefault(Addr, Base, Offset);
+      }
+    }
+
----------------
I think we still need to sink the alignment check into selectAddrFrameIndexOffset() to make this check robust.

In the current code, if selectAddrRegImmMSA() returns 'Base' as something other than a FrameIndexSDNode such as (add ptr, 3) then the current code will accept invalid offsets again. Admittedly, the current implementation of selectAddrRegImmMSA() and the functions it calls can only match FrameIndexSDNode nodes right now but we should improve that as soon as possible.



http://reviews.llvm.org/D19475