[PATCH] D23068: LoadStoreVectorizer: Remove TargetBaseAlign. Keep alignment for stack adjustments.

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:451
@@ -443,2 +450,3 @@
     return false;
+  }
 
----------------
We don't generally include commented-out code like this.  It makes things hard to read.

In this case it's particularly hard for me to read because there are two TODOs and it's not clear how they relate to one another.

Can we have just one comment that explains where we are today and where we want to be in the future?

================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:451
@@ -443,2 +450,3 @@
     return false;
+  }
 
----------------
jlebar wrote:
> We don't generally include commented-out code like this.  It makes things hard to read.
> 
> In this case it's particularly hard for me to read because there are two TODOs and it's not clear how they relate to one another.
> 
> Can we have just one comment that explains where we are today and where we want to be in the future?
LLVM style is to leave off braces on single-line "if" statements.  Unfortunately clang-tidy doesn't do this for you (such a waste of human time...).

================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:479
@@ -470,2 +478,3 @@
     return false;
+  }
 
----------------
I don't think you meant to include this change?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1038
@@ +1037,3 @@
+  DEBUG({
+    dbgs() << "LSV: Target said misaligned is allowed? " <<Allows<<" and fast? "<<Fast<<"\n";
+  });
----------------
Please run clang-format.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1038
@@ +1037,3 @@
+  DEBUG({
+    dbgs() << "LSV: Target said misaligned is allowed? " <<Allows<<" and fast? "<<Fast<<"\n";
+  });
----------------
jlebar wrote:
> Please run clang-format.
We don't usually use {} for DEBUGs that just contain a single log statement.  clang-format should be able to do something pretty here without the {}, I think.

================
Comment at: test/Transforms/LoadStoreVectorizer/X86/preserve-order32.ll:21
@@ -22,1 +20,3 @@
+  %buff.p = load i8*, i8** %tmp1, align 4
+  %buff.val = load i8, i8* %buff.p, align 4
   store i8 0, i8* %buff.p, align 8
----------------
Why the change on this line?  Maybe you don't need any align for this load?

================
Comment at: test/Transforms/LoadStoreVectorizer/X86/preserve-order64.ll:25
@@ -24,3 +24,3 @@
   %tmp0 = getelementptr inbounds %struct.buffer_t, %struct.buffer_t* %buff, i64 0, i32 0
-  %buff.int = load i64, i64* %tmp0, align 8
+  %buff.int = load i64, i64* %tmp0, align 16
   ret void
----------------
I thought x86 doesn't care about the alignment?  Or, is it a question of the "fast" param?


https://reviews.llvm.org/D23068