This is an archive of the discontinued LLVM Phabricator instance.

fix memcpy/memset/memmove lowering when optimizing for size
ClosedPublic

Authored by spatel on Jul 28 2015, 11:07 AM.

Details

Summary

Fixing MinSize attribute handling was discussed in D11363. This is a prerequisite patch to doing that.

The handling of OptSize when lowering mem* functions was broken on Darwin because it wants to ignore -Os for these cases, but the existing logic also made it ignore -Oz (MinSize).

The Linux change demonstrates a widespread problem. The backend doesn't recognize the MinSize attribute by itself; it assumes that MinSize implies OptSize. Fixing this more generally will be a follow-on patch or two.

Diff Detail

Event Timeline

spatel updated this revision to Diff 30837.Jul 28 2015, 11:07 AM
spatel retitled this revision from to fix memcpy/memset/memmove lowering when optimizing for size.
spatel updated this object.
spatel added reviewers: qcolombet, hfinkel, mkuper.
spatel added a subscriber: llvm-commits.
mkuper edited edge metadata.Jul 28 2015, 11:27 AM

Thanks for taking care of this, Sanjay.
It would be really nice to be consistent.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4156

Should start with a capital, I think.

4157

Same.

4159

Is this Darwin/non-Darwin distinction documented anywhere?
And do we respect it anywhere else, aside from the memfunc issue?

spatel added inline comments.Jul 28 2015, 11:57 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4156

Yep, will fix.

4159

AFAIK, it's not documented anywhere publicly. It goes back to the PowerPC days at Apple (so a bit strange that it's not also in the PPC lowering).

At the time, Apple built almost all internal projects with -Os and made that the default opt level for developers too (not sure if this is still true today).

The mem* lowering was an exception to the general rule to always optimize for size because it was clearly a win on some benchmark to inline those. So I assume this little hack was added to gcc and made it's way to LLVM too. :)

The existing test case references: rdar://8821501 Perhaps someone at Apple can give us an update. It may be that we don't even need to make this distinction for Darwin anymore?

Ie, on x86 an inline 'rep movs' might be a better answer, and the AArch64 backend does not currently make any concessions for size on any platform:

MaxStoresPerMemset = MaxStoresPerMemsetOptSize = 8;
MaxStoresPerMemcpy = MaxStoresPerMemcpyOptSize = 4;
MaxStoresPerMemmove = MaxStoresPerMemmoveOptSize = 4;
spatel updated this revision to Diff 30851.Jul 28 2015, 1:46 PM
spatel edited edge metadata.

Patch updated:
Fixed capitalization of variables.

spatel marked 3 inline comments as done.Jul 28 2015, 1:47 PM

Well, the code LGTM.

As to whether this is still the desired behavior - your call.
I'm ok with committing this as essentially NFC, and then ripping it out later if it turns out to be no longer necessary.

Well, the code LGTM.

As to whether this is still the desired behavior - your call.
I'm ok with committing this as essentially NFC, and then ripping it out later if it turns out to be no longer necessary.

Thanks, Michael.
Yes, my intention with this patch was to leave Darwin codegen as-is except to fix the bug of not producing smaller code at -Oz. Anything more should be a separate patch. Adding Jim Grosbach to see if we can get any clarification on that.

This revision was automatically updated to reflect the committed changes.