This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Be defensive when matching size_t in lib call signatures
ClosedPublic

Authored by bjope on Sep 27 2021, 1:41 PM.

Details

Summary

When TargetLibraryInfoImpl::isValidProtoForLibFunc is checking
function signatures to detect lib calls it may check that a parameter
or return value matches with the "size_t" type. For this to work it
has to derive the IR type matching with "size_t". Depending on if
a DataLayout is provided or not, this has been done in two different
way. Either a more strict check being based on IntPtrType (which is
given by the DataLayout) or a more relaxed check assuming that any
integer type matches with "size_t".

Given that the stricter approach exist it seems like we do not want
to trigger rewrites etc if we aren't sure that a function calls
actually match with the library function. Therefore it was questioned
why we actually have the more relaxed approach when not being able
to derive an IR type for "size_t". This patch will take a more
defensive approach, avoiding lib call transformations when we do not
know if the signature matches (when we do not know the size of
"size_t").

Diff Detail

Event Timeline

bjope created this revision.Sep 27 2021, 1:41 PM
bjope requested review of this revision.Sep 27 2021, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 1:41 PM
nikic added a reviewer: nikic.Sep 27 2021, 1:56 PM
nikic added a subscriber: nikic.

I think the "optional DataLayout" part here is just an unused leftover. If I add an assert(DL) in getLibFunc() (the only caller of isValidProtoForLibFunc()) I don't get any test failures. I'd recommend just making DataLayout explicitly required. Assert / make it a reference.

nikic added inline comments.Sep 27 2021, 2:03 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
734

As a side note, I believe this should really be using the integer type with width DL.getIndexSizeInBits(). Basically pointer size = intptr_t and index size = size_t.

bjope added inline comments.Sep 27 2021, 2:38 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
734

Ok. I did not know about "index size = size_t" relation.

(Makes me wonder about the index type in the DataLayout for our OOT target. We haven't configured that afaict, so we get index size = pointer size by default. But in our target size_t is different from intptr_t. So we should probably specify those optional params in the data layout to set the IndexSize. Will be interesting to see if that will impact something else.)

bjope updated this revision to Diff 375492.Sep 28 2021, 1:49 AM

Update after review feedback:
Now assuming that we always have a Module (and DataLayout) when using getLibFunc/isValidProtoForLibFunc.

This revision is now accepted and ready to land.Sep 28 2021, 1:53 AM
spatel accepted this revision.Sep 28 2021, 6:04 AM
This revision was landed with ongoing or failed builds.Sep 28 2021, 6:30 AM
This revision was automatically updated to reflect the committed changes.