This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Support for inlining memcpy, memset and memmove calls
ClosedPublic

Authored by aemerson on Jul 23 2019, 2:25 PM.

Details

Summary

This introduces a new family of combiner helper routines that re-use the target specific cost model from SelectionDAG, and generate inline implementations of the memcpy family of intrinsics.

The combines are only enabled at optimization levels higher than -O0, and give very substantial performance improvements.

Diff Detail

Event Timeline

aemerson created this revision.Jul 23 2019, 2:25 PM
paquette added inline comments.Jul 23 2019, 3:15 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
442

de morgan?

550–553

Could this part be pulled out into the caller? It shows up in each of the optimization functions and does the same thing. Then this could just assert on KnownLen?

580–586

I feel like this could be a helper, since it appears in a few of these functions?

692–712

Could this be a helper, since it appears in two of these functions?

825

Is this worth collecting a statistic for? Does SDAG have something like that for comparison?

838

If you init this to Src, then you can kill the if and make the else a if (CurrOffset != 0)?

Then you can also scope Offset into the new if.

880

initialize to false?

885–886

Should we just assert that we have one of these IDs instead of putting them in a switch?

907–909

Could probably check this before entering the switch

aemerson marked 8 inline comments as done.Jul 23 2019, 4:04 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
442

Done.

550–553

Sure.

580–586

I'm gonna disagree here because the blocks use 6 variables from the function scope, so by the time we plumb through all the arguments we're going to end up with a similar amount of code duplication, with additional indirection.

825

I don't think SelectionDAG does either. I can add it if required but these are so common I'm not sure what the benefit will be to have them.

838

Ok.

880

Ok.

885–886

I'll simplify it a bit.

907–909

Ok.

aemerson updated this revision to Diff 211375.Jul 23 2019, 4:15 PM

Addressed most of the comments.

paquette accepted this revision.Jul 24 2019, 10:13 AM

LGTM with some comments

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
428

s/an partial/a partial/

595

Can we have some documentation for what the memset is being optimized to and what the stores should look like?

(same for the other optimize functions)

880

IIRC this returns an int64_t and can return negative values; should we check that this is positive? Or is it not possible for it to be negative?

882–885

should we assert that KnownLen != 0 in the optimize functions now?

This revision is now accepted and ready to land.Jul 24 2019, 10:13 AM
aemerson marked 2 inline comments as done.Jul 24 2019, 3:02 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
880

I don't think it's possible to specify a "negative" length, if it somehow happened then it would be UB I think.

882–885

Will do.

This revision was automatically updated to reflect the committed changes.