This is an archive of the discontinued LLVM Phabricator instance.

[instcombine] Delete duplicate object size logic
ClosedPublic

Authored by reames on Jan 7 2022, 8:51 AM.

Details

Summary

InstCombine appears to duplicate the allocation size logic used inside getObjectSize when figuring out which attributes are safe to place on the callsite. We can use the existing utility function instead.

The test change is correct. With aligned_alloc, a zero alignment is required to return nullptr. As such, deref_or_null is a correct attribute to use.

Posting for review as the code implementing getObjectSize is a tad subtle, and I want to get a second set of eyes to make sure I'm not missing a more material difference.

Diff Detail

Event Timeline

reames created this revision.Jan 7 2022, 8:51 AM
reames requested review of this revision.Jan 7 2022, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2022, 8:51 AM
xbolva00 added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

all allocation fns have nonnull attribute, I believe.

reames added inline comments.Jan 7 2022, 8:58 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

Er, definitely not. Malloc can return nullptr on error. Did you maybe mean that all operator new versions did? I couldn't find that code, can you point me to it?

xbolva00 added inline comments.Jan 7 2022, 9:02 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

Yeah right, just for new calls.

Yes, todo is correct here for others

reames added inline comments.Jan 7 2022, 9:03 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

Your wording in last comment is unclear to me, can you rephrase?

xbolva00 added inline comments.Jan 7 2022, 9:15 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

That for throwing version of 'new' we already have nonnull.

I am wondering if just “deref(N)” attribute alone is enough for other parts of LLVM to assume also “nonnull” (for addr space 0)

reames added inline comments.Jan 7 2022, 9:20 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

Can you point me to the code which infers nonnull for the throwing operator new variants? I did not find it on a quick skim. In particular, I do not see this in BuildLibCalls.cpp which I would expect.

(Regardless, I'd like to leave this cleanup to a separate patch.)

No tests fail, so this looks good to go.

xbolva00 added inline comments.Jan 7 2022, 9:28 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578
reames added inline comments.Jan 7 2022, 9:45 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2578

Ah, I was looking in LLVM proper. Thanks!

Will cleanup the tests and remove this in a separate commit.

nikic accepted this revision.Jan 7 2022, 9:56 AM

LGTM

This revision is now accepted and ready to land.Jan 7 2022, 9:56 AM
This revision was landed with ongoing or failed builds.Jan 7 2022, 10:32 AM
This revision was automatically updated to reflect the committed changes.