This is an archive of the discontinued LLVM Phabricator instance.

MemCpyOpt cannot use ABI alignment even if it was not given
ClosedPublic

Authored by aqjune on Feb 5 2020, 12:02 PM.

Details

Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=44388 which incorrectly assigns an ABI alignment to memset when there was no explicit alignment given.

Event Timeline

aqjune created this revision.Feb 5 2020, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 12:02 PM
nikic added a comment.Feb 5 2020, 12:29 PM
define void @test(i32* nocapture %P) nounwind ssp {
entry:
  store i32 0, i32* %P
  %add.ptr = getelementptr inbounds i32, i32* %P, i64 1
  %0 = bitcast i32* %add.ptr to i8*
  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false)
  ret void
}

declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg)

This example no longer preserves the alignment (align 4 from ABI alignment on the store) after your patch. Possibly you need to capture that when populating Range.Alignment.

aqjune updated this revision to Diff 242732.Feb 5 2020, 12:48 PM
  • Let MemsetRanges::addStore explicitly record ABI alignment
nikic accepted this revision.Feb 5 2020, 1:04 PM

LG

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
178

I'd suggest moving the findStoreAlignment() helper below higher up in the file and use that, as it implements the same logic.

This revision is now accepted and ready to land.Feb 5 2020, 1:04 PM
aqjune updated this revision to Diff 242735.Feb 5 2020, 1:20 PM
  • Move find*Alignment up to the code & use it
This revision was automatically updated to reflect the committed changes.
gchatelet added inline comments.Feb 5 2020, 1:27 PM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
149

I believe this can now be written as

return DL.getValueOrABITypeAlignment(
    SI->getAlign(),
    SI->getOperand(0)->getType());
154

ditto

aqjune marked 3 inline comments as done.Feb 5 2020, 1:43 PM
aqjune added inline comments.