[PATCH] D69900: Align load/store for llvm.loop.aligned metadata

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

This seems to be only be used by the vectorizer. Are there plans for meanings beyond LoopVectorize? If not, it should be reflected in the metadata name: `llvm.loop.vectorize.aligned.enable`.

The semantics just assume that the data is aligned, but does not make it aligned. This should be reflected in the metadata name.

I do not see why an alignment of 32 fits everybody. Why not making it configurable, e.g. `{ !"llvm.loop.vectorize.assume_alignment", i64 32 }`?

A regression test is missing.



================
Comment at: llvm/docs/LangRef.rst:5443
 '``llvm.loop.vectorize.predicate.enable``' Metadata
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
----------------
Unrelated? 

Also, I think that reStructured Spinx warns if the underline is not long enough, and the bots are configured with fail-on-warnings.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:207-208
+
+  // /// Align all the array references
+  // bool alignArrayAccess(ArrayRef<Instruction *> Instrs, const DataLayout &DL);
 };
----------------
[nit] Please complete remove commented source


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:806
 
+
 /// Write a record \p DebugMsg about vectorization failure to the debug
----------------
[nit] unrelated


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7316
 
+void alignAccess(Loop *L,
+          LoopVectorizeHints &Hints){
----------------
Meinersbur wrote:
> [style] Please run clang-format over your code (`git clang-format HEAD`)
Since this function is only used withing this translation unit, make it static.

Also add a doxygen comment explaining what this function does.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7316-7317
 
+void alignAccess(Loop *L,
+          LoopVectorizeHints &Hints){
+
----------------
[style] Please run clang-format over your code (`git clang-format HEAD`)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7322
+  }
+  for(auto *BB : L->getBlocks()){
+    for(auto &I : *BB){
----------------
[style] The [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | LLVM coding standard ]] does not use almost-always-auto. 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7328-7329
+      if(SI){
+        // LLVM_DEBUG(dbgs()<<"Store "<<++st<<"\n");
+        // CreateAlignedLoad(SI->getType, SI->getPointerElementType(), 32, "aligned.load");
+        SI->setAlignment(32);
----------------
Please completely remove commented source


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69900/new/

https://reviews.llvm.org/D69900