SimplifyLibCalls: Optimize wcslen
ClosedPublic

Authored by MatzeB on Wed, May 3, 4:45 PM.

Details

Summary

Refactor the strlen optimization code to work for both strlen and wcslen.

This especially helps with programs in the wild where people pass L"string"s to const std::wstring& function parameters and the wstring constructor gets inlined.

Diff Detail

Repository
rL LLVM
MatzeB created this revision.Wed, May 3, 4:45 PM
MatzeB updated this revision to Diff 97758.Wed, May 3, 6:01 PM

Add some tests that I missed to include in the last revision.

efriedma added inline comments.
lib/Transforms/Utils/SimplifyLibCalls.cpp
532 ↗(On Diff #97758)

Making assumptions about the implementation of wcslen() based on the type of the pointer is bad idea. It probably works most of the time for code generated by clang, but pointee types don't have any semantics in IR, so it's likely to mess up at some point. (Also, pointee types in IR are likely to go away at some point in the future.)

I think the right approach is to add a function to TargetLibraryInfo to compute the size of wchar_t based on the triple.

MatzeB added inline comments.Thu, May 4, 11:45 AM
lib/Transforms/Utils/SimplifyLibCalls.cpp
532 ↗(On Diff #97758)

I assume the argument must match the declaration of wcslen which I assumed to be correct. I could grab the information directly from the declaration to avoid problems when the IR pointers don't have pointee-types anymore.

I'm not convinced the Triple/DataLayout has enough information to give you the type of wchar_t. There are for example the -fshort-wchar and -fno-short-wchar clang flags that do not affect the triple or datalayout.

efriedma added inline comments.Thu, May 4, 12:15 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
532 ↗(On Diff #97758)

If we want to support compiling against a C library built with "-fshort-wchar" on a platform where wchar_t is 32 bits by default, the triple isn't sufficient, yes. We can emit that information from clang; see http://llvm.org/docs/LangRef.html#c-type-width-module-flags-metadata . (We currently only emit it on ARM, but we could emit it on other architectures.)

MatzeB updated this revision to Diff 97871.Thu, May 4, 1:45 PM
  • Determine wchar_t size based on the wcslen declaration
  • Improve/fix tests
MatzeB marked 3 inline comments as done.Thu, May 4, 1:48 PM
MatzeB added inline comments.
lib/Transforms/Utils/SimplifyLibCalls.cpp
532 ↗(On Diff #97758)

The updated version gets wchar_t by looking at the wcslen declaration. I think that is in line with other code here (the length of size_t is also determined by looking at the call result types / function declarations if you look around).

(I don't think adding support for determining the sizes of C/C++ types into TargetLibraryInfo is a worthwhile goal at this point.)

dblaikie added inline comments.Thu, May 4, 2:00 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
532 ↗(On Diff #97871)

Any call to "getPointerElementType" won't be valid post-typeless-pointer work. Pointers will be opaque blobs (think of it as if your C code was "all void* all the time"). Please avoid adding uses of a pointer's element type where practical (using it to determine if bitcasts need to be added, etc is unavoidable for now - and would go away once typeless pointers are done and pointer bitcasts are no longer meaningful/needed).

So, yes, I'm not sure how this could be done correctly in that case without some kind of target hint/info about the size of a wchar?

efriedma added inline comments.Thu, May 4, 2:16 PM
lib/Analysis/ValueTracking.cpp
3038 ↗(On Diff #97871)

Do we care about the size of the array in this case?

3069 ↗(On Diff #97871)
lib/Transforms/Utils/SimplifyLibCalls.cpp
458 ↗(On Diff #97871)

Could you use an iterator which returns the elements of the ConstantDataArray, instead of using explicit indexing everywhere?

MatzeB updated this revision to Diff 98005.Fri, May 5, 1:04 PM
MatzeB marked an inline comment as done.
  • Add functionality to TargetLibraryInfo to determine the size of wchar_t for a target triple (an upcoming clang patch will introduce an assert() on the clang side to catch when they become out of sync).
  • getConstantDataArrayInfo() now returns Offset+Length into the ConstantDataArray and corrects handling of zeroinitializers
MatzeB marked 4 inline comments as done.Fri, May 5, 1:08 PM
MatzeB added inline comments.
lib/Analysis/ValueTracking.cpp
3038 ↗(On Diff #97871)

Looks like you answered your own question by digging out that PR in the next comment :)
That testcase is fixed as well in my latest revision.

lib/Transforms/Utils/SimplifyLibCalls.cpp
458 ↗(On Diff #97871)

There are no iterators defined for ConstantDataArray, did I miss something? (Defining them seems tricky to get right to support various datatypes and target/host data conversions).

MatzeB marked an inline comment as done.Fri, May 5, 1:09 PM
MatzeB added a subscriber: olista01.
efriedma added inline comments.Fri, May 5, 1:41 PM
lib/Analysis/TargetLibraryInfo.cpp
1420 ↗(On Diff #98005)

We should probably have a verifier check to make sure these casts will succeed.

lib/Analysis/ValueTracking.cpp
3043 ↗(On Diff #98005)

Do you need to check the type against ElementSize here?

lib/Transforms/Utils/SimplifyLibCalls.cpp
458 ↗(On Diff #97871)

No, there aren't any existing iterators.

It doesn't seem that tricky to write, though. It could just be a wrapper around getElementAsInteger; or if you want to get fancy, you could represent it as a pointer+width pair, I guess. (The data is just an array of integers in host byte order.)

MatzeB updated this revision to Diff 98206.Mon, May 8, 2:08 PM
MatzeB marked 3 inline comments as done.
  • Check type in !Array zeroinitializer case. (Technically this shouldn't be necessary as it would be UB otherwise, but optimizing based on this will hardly buy us anything).
lib/Analysis/TargetLibraryInfo.cpp
1420 ↗(On Diff #98005)
lib/Transforms/Utils/SimplifyLibCalls.cpp
458 ↗(On Diff #97871)

I tried this and it looks like this: https://reviews.llvm.org/P7994
39 extra lines added for the iterator foo and none of the actual uses gets simpler.
(If you now want to argue that I didn't use llvm::enumerate(), then I'd kindly ask you to present me an example of how the heck I can implement an interator compliant the forward iterator concept where operator* returns a reference! to the underlying object).
I worked on this stuff for >2 hours now and it is definitely "tricky to write" and not really necessary here.

clang only emits wchar_size on ARM at the moment; that might need to change before this gets merged.

Missing tests for 16-bit wchar_t.

lib/Transforms/Utils/SimplifyLibCalls.cpp
458 ↗(On Diff #97871)

Mmm... okay, maybe not as simple as I'd hoped; fine.

MatzeB updated this revision to Diff 99608.Fri, May 19, 11:40 AM
  • Add a test passing a pointer to an array with the wrong size to wcslen.
  • Add variants of tests in wcslen-1.ll in wcslen-3.ll that test with sizeof(wchar_t)==2.
  • (The clang patch to always emit wchar_size is approved.)
This revision is now accepted and ready to land.Fri, May 19, 12:10 PM

Fixing getConstantStringInfo() here caused some changes in behavior in SelectionDAG memcpy lowering. I'll put in some more fixes before committing...

This revision was automatically updated to reflect the committed changes.