This is an archive of the discontinued LLVM Phabricator instance.

Align load/store for llvm.loop.aligned metadata
Needs ReviewPublic

Authored by m-happy on Nov 6 2019, 9:01 AM.

Details

Summary

This patch adds functionality 'llvm.loop.aligned' metadata. The load/store accesses are aligned to 32 bit for loops marked with 'llvm.loop.aligned'.
This is used by D69897

Diff Detail

Event Timeline

m-happy created this revision.Nov 6 2019, 9:01 AM

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.

llvm/docs/LangRef.rst
5443

Unrelated?

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

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
207–208

[nit] Please complete remove commented source

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
806

[nit] unrelated

7316

Since this function is only used withing this translation unit, make it static.

Also add a doxygen comment explaining what this function does.

7316–7317

[style] Please run clang-format over your code (git clang-format HEAD)

7322

[style] The LLVM coding standard does not use almost-always-auto.

7328–7329

Please completely remove commented source

Do we need this at all? Can't we just emit @llvm.assume calls before the loop if we know the alignment? I've done this in the past and it's worked well (by using __builtin_assume_aligned from Clang).

jdoerfert added a subscriber: jdoerfert.EditedNov 6 2019, 2:00 PM

Do we need this at all? Can't we just emit @llvm.assume calls before the loop if we know the alignment? I've done this in the past and it's worked well (by using __builtin_assume_aligned from Clang).

I'm in favor of doing this (llvm.assume) in the frontedn so other passes can use the information as well without requiring them to understand yet another encoding.