[PATCH] Take alignment into account for speculative loads

Sanjoy Das sanjoy at playingwithpointers.com
Wed Jun 3 19:09:30 PDT 2015


Some comments inline.

This change is still too large, do you think it makes sense to separate out the changes to `lib/Analysis/Loads.cpp`?

You should also make sure you can bootstrap clang with this change in place.


================
Comment at: include/llvm/Analysis/ValueTracking.h:231
@@ +230,3 @@
+
+  /// Returns true if this is always a dereferenceable pointer with alignment
+  /// greater or equal than requested. If the context instruction is specified
----------------
Nit: `this` should be `V`.

================
Comment at: lib/Analysis/Loads.cpp:68
@@ -67,1 +67,3 @@
   const DataLayout &DL = ScanFrom->getModule()->getDataLayout();
+
+  // Require ABI alignment for loads without alignment specification
----------------
I'd put an assert here that `Align` is a power of 2.

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:26
@@ -26,1 +25,3 @@
+    SmallVector<Value *, 4> Deref;
+    std::set<Value *> DerefAndAligned;
 
----------------
Use `DenseSet` or `SmallPtrSet`.

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:72
@@ -67,1 +71,3 @@
     V->print(OS);
+    if (DerefAndAligned.find(V) == DerefAndAligned.end())
+      OS << "\t(unaligned)";
----------------
Use `.count` (both `DenseSet` and `SmallPtrSet` have it).

================
Comment at: lib/Analysis/ValueTracking.cpp:2927
@@ +2926,3 @@
+  APInt BaseAlign(Offset.getBitWidth(), 0);
+  if (const AllocaInst *AI = dyn_cast<AllocaInst>(Base)) {
+    BaseAlign = AI->getAlignment();
----------------
The braces are unnecessary here.

================
Comment at: lib/Analysis/ValueTracking.cpp:3003
@@ -2976,3 +3002,3 @@
       return false;
-    if (!isDereferenceablePointer(Base, DL, CtxI,
-                                  DT, TLI, Visited))
+    if (!isDereferenceableAndAlignedPointer(Base, Align, DL, CtxI, DT, TLI,
+                                            Visited))
----------------
Why do we have a restriction on `Base`s alignment here?  We check below that `Base + Offset` is properly aligned, right?  I'm not strongly opposed to keeping it like this, but please add a comment.

http://reviews.llvm.org/D9791

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list