This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Deduce attributes for aligned_alloc in InstCombine
ClosedPublic

Authored by bondhugula on Mar 27 2020, 11:40 PM.

Details

Summary

Make InstCombine aware of the aligned_alloc library function.

Signed-off-by: Uday Bondhugula <uday@polymagelabs.com>

Depends on D76970.

Diff Detail

Event Timeline

bondhugula created this revision.Mar 27 2020, 11:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 11:40 PM
jdoerfert added inline comments.Mar 27 2020, 11:43 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

When Op0C is set (regardless of Op1C), we can set the alignment attribute on the return value, can't we?

bondhugula added inline comments.Mar 28 2020, 12:13 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

Would it be correct to set the alignment attribute for 0 sized allocations?

jdoerfert added inline comments.Mar 28 2020, 12:32 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

Would it be correct to set the alignment attribute for 0 sized allocations?

Not in general, but: The function aligned_alloc() is the same as memalign(), except for the added restriction that size should be a multiple of alignment. So for a non-zero alignment size need to be non-zero. Please correct me if I'm wrong though.

bondhugula added inline comments.Mar 28 2020, 1:39 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

The size could still be zero. (zero as size would be a multiple of any alignment. :-))

bondhugula marked 4 inline comments as done.Mar 28 2020, 3:32 AM
jdoerfert added inline comments.Mar 30 2020, 7:56 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

The size could still be zero. (zero as size would be a multiple of any alignment. :-))

True, it was late.

Then remove the "(regardless of Op1C),") from my original comment. We still can and should add alignment information.

llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
39

I mean, we can add align 32 here can't we not?

bondhugula added inline comments.Mar 30 2020, 9:02 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
4445

Sure, we should. I was just making sure under what circumstances we could. Will add this, thanks.

llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
39

Absolutely!

Address review comments; add alignment attribute to aligned_alloc sites

bondhugula marked 4 inline comments as done.Mar 30 2020, 10:46 AM

Thanks for the review, @jdoerfert Done.

add attribute only if the alignment is a power of two

xbolva00 added inline comments.
llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
37

aligned_alloc(i64 %align, i64 512)
aligned_alloc(i64 0, i64 512)

Please add such tests

bondhugula marked 2 inline comments as done.

More test cases.

llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
37

Thanks!

xbolva00 accepted this revision.Mar 30 2020, 1:59 PM

Looks good to me. Wait a day or so before landing.

This revision is now accepted and ready to land.Mar 30 2020, 1:59 PM
jdoerfert accepted this revision.Mar 30 2020, 3:06 PM

LGTM as well. Thx and sorry for the confusion.

bondhugula retitled this revision from Deduce attributes for aligned_alloc in InstCombine to [InstCombine] Deduce attributes for aligned_alloc in InstCombine.Mar 31 2020, 10:48 AM
This revision was automatically updated to reflect the committed changes.