This is an archive of the discontinued LLVM Phabricator instance.

Use getMostFrequentByte to decide if should used memset+stores
Changes PlannedPublic

Authored by vitalybuka on Jul 8 2019, 5:22 PM.

Details

Reviewers
eugenis
pcc
jfb
Summary

Previous implementation handled only memsets to zero.
This patch keeps behavior as close as possible to the original.
In followup patches I'll resolve TODOs and update/add tests.

Significantly changed part of @pcc prototype for -ftrivial-auto-var-init=pattern optimizations

Event Timeline

vitalybuka created this revision.Jul 8 2019, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 5:22 PM
vitalybuka planned changes to this revision.Jul 8 2019, 5:23 PM
vitalybuka edited the summary of this revision. (Show Details)Jul 8 2019, 8:04 PM
vitalybuka updated this revision to Diff 208583.Jul 8 2019, 8:17 PM

Reverted debug logging

jfb added inline comments.Jul 8 2019, 9:29 PM
clang/lib/CodeGen/CGDecl.cpp
865

You should update the return value to Optional<Value*> where the non-None return is the bytewise value.

873

I'm not sure I understand what this bit does. If something isn't a bytewise value then we now automatically assume it can be stored to with a single store. That's not necessarily true. I think you need to drop BWInit || from here, and conditionalize this entire branch with if (BWInit) instead (so, an && on it all).

888

The recursion now allows different values for BWInit on each sub-element.

895

What does this new loop do? It'll only ever go around the loop once.

vitalybuka updated this revision to Diff 208594.Jul 8 2019, 9:31 PM

undo some behavior change

vitalybuka marked 4 inline comments as done.Jul 9 2019, 2:41 AM
vitalybuka added inline comments.
clang/lib/CodeGen/CGDecl.cpp
865

Why?
We already know value which was used for memset, now we just counting additional store/memsets

873

If value is bytewise, we will emit BWInit

888

yes

895

it counts number of stores used by elements
similar look we had in canEmitInitWithFewStoresAfterBZero

vitalybuka marked an inline comment as done.Jul 9 2019, 3:09 AM
vitalybuka added inline comments.
clang/lib/CodeGen/CGDecl.cpp
873

If value is bytewise, we will emit BWInit

If value is bytewise, we will emit memset, which we can count as one store.
It's not always true that memset costs as a single store. However this is a heuristic, as the limit 6 itself.

Here I try to keep behavior close to original for zeroes.
In future patches I'd like to try to remove this function and use just "density" ((count memset bytes that match initializer)/(full size of initializer)). This will be simpler, should work good enough for exiting cases, and cover new cases with large initializers where 6 is not enough,

vitalybuka edited the summary of this revision. (Show Details)Jul 10 2019, 4:05 PM
vitalybuka planned changes to this revision.May 24 2023, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 5:17 PM
Herald added a subscriber: nlopes. · View Herald Transcript