This is an archive of the discontinued LLVM Phabricator instance.

MemoryBuiltins: remove malloc-family funcs from list
ClosedPublic

Authored by durin42 on Jul 19 2022, 11:57 AM.

Details

Summary

We no longer need specialized knowledge of these allocator functions in
this file since we have the correct attributes available now.

As far as I can tell the changes in the attributor tests are due to
things getting more consistent on alloc-family once we remove the static
list entries.

The two test changes in NewGVN merit extra scrutiny: NewGVN appears to
be _extremely_ sensitive to the inaccessiblememonly for reasons that
are beyond me. As a result, I had-enumerated all the attributes on
allocation functions in those two tests instead of using -inferattrs.
I assumed that the two -disable-simplify-libcalls tests there no
longer are sensible since the function declaration now includes all the
relevant attributes.

Diff Detail

Event Timeline

durin42 created this revision.Jul 19 2022, 11:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 11:57 AM
durin42 requested review of this revision.Jul 19 2022, 11:57 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
durin42 updated this revision to Diff 446208.Jul 20 2022, 10:35 AM
durin42 updated this revision to Diff 446564.Jul 21 2022, 10:24 AM
durin42 edited the summary of this revision. (Show Details)
nikic added a subscriber: nikic.Jul 22 2022, 1:09 AM
nikic added inline comments.
llvm/test/Transforms/Attributor/value-simplify-pointer-info.ll
8

For this test, can we also add explicit attributes instead of running inferattrs?

arichardson added inline comments.
llvm/test/Transforms/NewGVN/calloc-load-removal.ll
16–17

Remove?

llvm/test/Transforms/NewGVN/malloc-load-removal.ll
25–26

These lines should probably be removed since the RUN line is gone now?

durin42 updated this revision to Diff 447398.Jul 25 2022, 10:20 AM
durin42 marked 3 inline comments as done.Jul 25 2022, 10:20 AM
nikic accepted this revision.Jul 25 2022, 11:47 AM

LGTM

This revision is now accepted and ready to land.Jul 25 2022, 11:47 AM
This revision was landed with ongoing or failed builds.Jul 25 2022, 2:29 PM
This revision was automatically updated to reflect the committed changes.
qiongsiwu1 added a subscriber: qiongsiwu1.EditedJul 28 2022, 7:00 AM

May I ask for some guidance/example on how to use the attributes to identify that the removed functions (e.g. malloc, calloc, realloc) are still allocation functions?

Is it true that bool llvm::isAllocationFn returns false now if the input is a malloc call? It seems to be so in my experiments. Is this intended? This is contradictory to the code comment .

Additionally, what is the recommended approach now to tell if a library function allocates memory? It seems that bool llvm::isAllocationFn is covering some functions (ones still included in the list AllocationFnData), but not others (ones this patch removed). When should we use bool llvm::isAllocationFn? Or should we migrate to use attributes and avoid using bool llvm::isAllocationFn?

Thanks so much!!

May I ask for some guidance/example on how to use the attributes to identify that the removed functions (e.g. malloc, calloc, realloc) are still allocation functions?

Is it true that bool llvm::isAllocationFn returns false now if the input is a malloc call? It seems to be so in my experiments. Is this intended? This is contradictory to the code comment .

If the function isn't annotated with the right attributes, it won't be understood to be an allocation function unless it's in the big static list in MemoryBuiltins. There were a series of changes to BuildLibCalls (eg D123089) which mean that the -inferattrs pass will insert the right attributes on your function declarations, so you may just need an extra pass for that to work, or to add the attributes manually in your test case if you don't want to run attribute inference.

Additionally, what is the recommended approach now to tell if a library function allocates memory? It seems that bool llvm::isAllocationFn is covering some functions (ones still included in the list AllocationFnData), but not others (ones this patch removed). When should we use bool llvm::isAllocationFn? Or should we migrate to use attributes and avoid using bool llvm::isAllocationFn?

I'd still use isAllocationFn - that checks both the static list and the attributes. If we ever manage to get the list to empty, that should still work.

Does that help?

qiongsiwu1 added a comment.EditedJul 28 2022, 7:37 AM

Does that help?

Yes! I am trying out the inferattrs pass. Still having some test failures but as you mentioned the tests most likely need some tuning.

Thanks for the fast response!