This is an archive of the discontinued LLVM Phabricator instance.

[ExpandMemCmp] Correctly set alignment of generated loads
ClosedPublic

Authored by aqjune on Mar 12 2020, 6:10 PM.

Details

Summary

This is a part of the series of efforts for correcting alignment of memory operations.
(Another related bugs: https://bugs.llvm.org/show_bug.cgi?id=44388 , https://bugs.llvm.org/show_bug.cgi?id=44543 )

This fixes https://bugs.llvm.org/show_bug.cgi?id=43880 by giving default alignment of loads to 1.

The test CodeGen/AArch64/bcmp-inline-small.ll should have been changed; it was introduced by https://reviews.llvm.org/D64805 . I talked with @evandro, and confirmed that the test is okay to be changed.
Other two tests from PowerPC needed changes as well, but fixes were straightforward.

Diff Detail

Event Timeline

aqjune created this revision.Mar 12 2020, 6:10 PM
aqjune updated this revision to Diff 250126.Mar 12 2020, 10:20 PM

Update test Transforms/ExpandMemCmp/X86/memcmp.ll

Can you please add a test with an explicit parameter alignment ?

courbet added inline comments.Mar 16 2020, 12:40 AM
llvm/test/CodeGen/AArch64/bcmp-inline-small.ll
38–41

I'm not sure what this checks exactly.
Can you expand the checked pattern ? It would make it clearer what we're looking for.

aqjune updated this revision to Diff 250495.Mar 16 2020, 1:53 AM
  • Add tests that have alignments initially given to fn args
  • Let alignment be successfully inferred when gep is involved
aqjune marked an inline comment as done.Mar 16 2020, 1:55 AM
aqjune added inline comments.
llvm/test/CodeGen/AArch64/bcmp-inline-small.ll
38–41

It checks whether ldrb is repeated 30 times (for reading 15 bytes of %s1 and %s2 each).
I think expanding this would be a bit verbose; I attach the output asm file.

aqjune added a subscriber: nlopes.Mar 16 2020, 1:57 AM
courbet added inline comments.Mar 16 2020, 2:09 AM
llvm/lib/CodeGen/ExpandMemCmp.cpp
286

Because alignments are always powers of two, assuming n<m, gcd(2^n, 2^m) is the same as 2^n * gcd(1, 2^(m-n)), or 2^n, i.e. min(2^n, 2^m). So please use Align commonAlignment(Align A, uint64_t Offset) from Alignment.h

llvm/test/CodeGen/AArch64/bcmp-inline-small.ll
38–41

ping

llvm/test/Transforms/ExpandMemCmp/X86/memcmp.ll
25

Please submit the base tests to show the diffs.

courbet accepted this revision.Mar 16 2020, 2:14 AM

LGTM when the comments are addressed.

llvm/test/CodeGen/AArch64/bcmp-inline-small.ll
38–41

OK, that's what I suspected (this behaviour of Filecheck is undocumented and I coul dnot find similar usages). Well, that's an issue because this blows up the number of loads, which is not really desirable (@evandro FYI). But this is better than broken code, so LGTM.

This revision is now accepted and ready to land.Mar 16 2020, 2:14 AM

Thanks. I am temporarily out of office, will be back and address comments soon

gchatelet added inline comments.Mar 16 2020, 3:33 AM
llvm/lib/CodeGen/ExpandMemCmp.cpp
276

LhsSource->getPointerAlignment(DL).valueOrOne()
and below as well.

aqjune updated this revision to Diff 250547.Mar 16 2020, 6:22 AM

Address comments

aqjune updated this revision to Diff 250551.Mar 16 2020, 6:38 AM
aqjune marked 2 inline comments as done.

diff from precommitted test

aqjune marked 2 inline comments as done.Mar 16 2020, 6:41 AM
aqjune added inline comments.
llvm/test/CodeGen/AArch64/bcmp-inline-small.ll
38–41

It is '“CHECK-COUNT:” directive' at FileCheck doc (-COUNT was needed to make it correctly work, I fixed it)

This revision was automatically updated to reflect the committed changes.