This is an archive of the discontinued LLVM Phabricator instance.

InstructionCombining: merge realloc and free handling
AbandonedPublic

Authored by durin42 on Apr 4 2022, 10:45 AM.

Details

Reviewers
jyknight
nikic
Summary

As best I can tell, all this code _actually_ cares about is that it's a
function from an allocator family, not that it's specifically realloc or
free (which are the only options anyway). We can check for the allocator
family instead of looking at "is realloc like" and "is free like"
separately. The downside is that we need to add the free() codepath to
the work list, but since it returns nothing that should be
inconsequential in terms of cost.

Diff Detail

Event Timeline

durin42 created this revision.Apr 4 2022, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 10:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
durin42 requested review of this revision.Apr 4 2022, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 10:45 AM
durin42 updated this revision to Diff 420851.Apr 6 2022, 7:41 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 420912.Apr 6 2022, 9:10 AM
durin42 updated this revision to Diff 421205.Apr 7 2022, 7:36 AM
durin42 updated this revision to Diff 422240.Apr 12 2022, 8:32 AM
durin42 updated this revision to Diff 424966.Apr 25 2022, 10:55 AM
nikic added a subscriber: nikic.

I don't think this change is semantically right. For example, the function could now also be an allocator function that just happens to accept a pointer argument -- say, an allocator that allocates into a memory region. I think we do need to explicitly check that it's a free/realloc.

What's the motivation for this change?

durin42 abandoned this revision.Apr 25 2022, 5:15 PM

I don't think this change is semantically right. For example, the function could now also be an allocator function that just happens to accept a pointer argument -- say, an allocator that allocates into a memory region. I think we do need to explicitly check that it's a free/realloc.

What's the motivation for this change?

Oh, great point. This was an unconsidered leftover from a previous incarnation of this (before allockind() existed) and it can be replaced by D124425 (which comes later in the sequence). I'll mark this as abandoned. Thanks!