[PATCH] Add address space argument to allowsUnalignedMemoryAccess.

Quentin Colombet qcolombet at apple.com
Tue Jan 21 10:29:22 PST 2014


  Hi Matt,

  The overall fix goes in the right direction, but you have a couple of typos in the subclasses when you overload the allowsUnalignedMemoryAccesses.

  I am not in favor of the change of order for the Fast argument in the modified API.
  Indeed, because of that change, every call that uses the Fast argument will need to be updated (like you did for the ARM backend).
  Therefore, I would prefer to have:
  virtual bool allowsUnalignedMemoryAccesses(EVT, bool * /*Fast*/ = 0, unsigned AddrSpace = 0) const

  That said, I understand the form you are proposing is more convenient for your target, thus if the majority agrees for the order change, I can cope with that :).

  Thanks,

  Quentin


================
Comment at: include/llvm/Target/TargetLowering.h:715
@@ -714,2 +714,3 @@
   ///
   /// This function returns true if the target allows unaligned memory accesses.
+  /// of the specified type in the given address space. If true, it also returns
----------------
Since you are updating the command, you could fix the typo here :).
The sentence is not finished here, thus the period should be removed.

================
Comment at: include/llvm/Target/TargetLowering.h:717
@@ +716,3 @@
+  /// of the specified type in the given address space. If true, it also returns
+  /// whether the unaligned memory access is "fast" in the second argument by
+  /// reference. This is used, for example, in situations where an array
----------------
if we decide to go with your API change:
s/second/third/

================
Comment at: include/llvm/Target/TargetLowering.h:719
@@ +718,3 @@
+  /// reference. This is used, for example, in situations where an array
+  /// copy/move/set is converted to a sequence of store operations. It's use
+  /// helps to ensure that such replacements don't generate code that causes an
----------------
I believe.
s/It’s/Its/

================
Comment at: lib/Target/Mips/MipsSEISelLowering.h:34
@@ -34,1 +33,3 @@
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast,
+                                               unsigned AS = 0) const;
 
----------------
This prototype does not match the base class.
You’ve inverted the order of bool and unsigned.

================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:249
@@ -248,1 +248,3 @@
+                                                    bool *Fast,
+                                                    unsigned) const {
   MVT::SimpleValueType SVT = VT.getSimpleVT().SimpleTy;
----------------
Same here.


http://llvm-reviews.chandlerc.com/D2522



More information about the llvm-commits mailing list