[PATCH] D80276: [Alignment] Fix misaligned interleaved loads

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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 ;)



================
Comment at: llvm/lib/Target/X86/X86InterleavedAccess.cpp:228
   }
 }
 
----------------
Nit: msg for the assert.



================
Comment at: llvm/test/Transforms/InterleavedAccess/X86/interleaved-accesses-64bits-avx.ll:45
 ; CHECK-NEXT:    [[TMP2:%.*]] = getelementptr <4 x i64>, <4 x i64>* [[TMP1]], i32 0
-; CHECK-NEXT:    [[TMP3:%.*]] = load <4 x i64>, <4 x i64>* [[TMP2]], align 16
+; CHECK-NEXT:    [[TMP3:%.*]] = load <4 x i64>, <4 x i64>* [[TMP2]], align 32
 ; CHECK-NEXT:    [[TMP4:%.*]] = getelementptr <4 x i64>, <4 x i64>* [[TMP1]], i32 1
----------------
nlopes wrote:
> efriedma wrote:
> > craig.topper wrote:
> > > jdoerfert wrote:
> > > > lebedev.ri wrote:
> > > > > nlopes wrote:
> > > > > > jdoerfert wrote:
> > > > > > > nlopes wrote:
> > > > > > > > craig.topper wrote:
> > > > > > > > > The original load was only align 16 how did we get to 32?
> > > > > > > > this is probably the ABI alignment for the vector is 32 bytes(?) and the code now will just take that.
> > > > > > > But it shouldn't. (Now it's gone so OK)
> > > > > > 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.
> > > > > So you are saying that every underaligned (incl. align=1) load is undefined?
> > > > > That is not my reading of langref..
> > > > I'm usually not leaving the IR and that is how I understood IR semantics. @arsenm @efriedma 
> > > > thoughts?
> > > 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.
> > 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.
> 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.
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 ;) )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80276