Page MenuHomePhabricator

[Alignment] Fix misaligned interleaved loads
ClosedPublic

Authored by gchatelet on May 20 2020, 12:43 AM.

Diff Detail

Event Timeline

gchatelet created this revision.May 20 2020, 12:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 12:43 AM
craig.topper added inline comments.May 20 2020, 1:16 AM
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

The original load was only align 16 how did we get to 32?

nlopes added inline comments.May 20 2020, 6:58 AM
llvm/lib/Target/X86/X86InterleavedAccess.cpp
226

at least iteration at 0 you could take LI->getAlign(), as it might be larger than ABI alignment. Propagating this larger alignment to subsequent loads requires a bit more code; not sure it's worth it.

llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

this is probably the ABI alignment for the vector is 32 bytes(?) and the code now will just take that.

nlopes added a subscriber: aqjune.May 20 2020, 6:59 AM
gchatelet updated this revision to Diff 265227.May 20 2020, 7:03 AM
  • Address comment
gchatelet marked an inline comment as done.May 20 2020, 7:03 AM
gchatelet marked 2 inline comments as done.May 20 2020, 7:06 AM
gchatelet added inline comments.
llvm/lib/Target/X86/X86InterleavedAccess.cpp
226

Just updated the code, I think it's what you were suggesting.

gchatelet marked 2 inline comments as done.May 20 2020, 7:19 AM
gchatelet added inline comments.
llvm/lib/Target/X86/X86InterleavedAccess.cpp
217

I thought TypeSize would be Bytes but it's Bits, fix is coming.

gchatelet updated this revision to Diff 265236.May 20 2020, 7:26 AM
  • Fixed Bits vs Bytes error
jdoerfert added a comment.EditedMay 20 2020, 8:10 AM

I think there is (maybe in some differential) a way to ask GEPOperator how much alignment they preserve. We could use that here.

EDIT: Found it: D79650 https://reviews.llvm.org/D79650#change-8Fft6F2LARjw

llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

But it shouldn't. (Now it's gone so OK)

nlopes added inline comments.May 20 2020, 9:21 AM
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

I don't think there's any problem in increasing alignment. All data needs to be aligned at least by the ABI requirement. Alignment given in memory instructions is only meaningful if greater than the minimum that the ABI specifies.

lebedev.ri added inline comments.
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

So you are saying that every underaligned (incl. align=1) load is undefined?
That is not my reading of langref..

jdoerfert added inline comments.
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

I'm usually not leaving the IR and that is how I understood IR semantics. @arsenm @efriedma
thoughts?

craig.topper added inline comments.May 20 2020, 10:22 AM
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

The vector load would have been created from scalar loads by the loop vectorizer. The only alignment the loop vectorizer saw was the scalar load. And whatever better alignment might have been inferred. There's no guarantee that a vector load created by the vectorizer would be aligned to anything more than the scalar.

Please regenerate the IR tests and commit that separately.

llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

The known alignment of a memory operation is precisely the specified alignment, no more or less. This has always been true. There are some cases where a "zero" alignment has a special meaning, but I've been working on eradicating that.

And yes, realistically, there isn't any reason to expect that vector loads created by the vectorizer are naturally aligned.

nlopes added inline comments.May 21 2020, 10:38 AM
llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

Ok, I've conducted some tests with Alive and I couldn't find any evidence that LLVM increases alignment beyond what's specified in the memory operations.
You guys are right in that this how LangRef reads. LangRef only authorizes use of ABI alignment if none is specified.

I guess my confusion comes from seeing in the past increases in alignment of global variables (-globalopt?), but that's different.
Anyway, apologies for the confusion.

I'm confused about the bug report. Did we just not check the alignment in the test and therefore we don't see the wrong alignment go away?

I think the code looks good now. I suggest someone can accept this ;)

llvm/lib/Target/X86/X86InterleavedAccess.cpp
229

Nit: msg for the assert.

llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll
45 ↗(On Diff #265156)

No worries!

I'm just glad we seem to be in agreement on the semantics we "want"/"have". That is good (and not always the case ;) )

I'm confused about the bug report. Did we just not check the alignment in the test and therefore we don't see the wrong alignment go away?

I think the code looks good now. I suggest someone can accept this ;)

If I rerun the script on the test in trunk, the bad alignments get added. Was this pass original producing alignment of 0 and then changed to have this bug?

If I rerun the script on the test in trunk, the bad alignments get added. Was this pass original producing alignment of 0 and then changed to have this bug?

I expect that the testcases originally had zero alignment, and and we propagated that zero alignment, and therefore appeared to do something sane. With my recent changes, we now fix up the alignment in IR parsing, and therefore propagate the alignment as an actual number. (This is why I suggested the testcases should be regenerated and committed separately, so it's clear what this patch is actually changing.)

Either way, the underlying bug isn't new. In practice, the alignment would never be zero at the point in a normal pass pipeline, even before my recent changes.

craig.topper accepted this revision.May 25 2020, 11:51 AM

Thanks @efriedma that probably explains it.

LGTM

This revision is now accepted and ready to land.May 25 2020, 11:51 AM
gchatelet updated this revision to Diff 266494.May 27 2020, 5:13 AM
gchatelet marked 9 inline comments as done.

rebase to integrate D80549

This revision was automatically updated to reflect the committed changes.

arc adds many unneeded tags from Phabricator. You can drop Reviewers: Subscribers: Tags: and the text Summary: with the following script:

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You should keep the tag. (I started to use --date=now because some people find author date != committer date annoying. The committer date is usually what people care.))