This is an archive of the discontinued LLVM Phabricator instance.

[X86] Constant folding of adds/subs intrinsics
ClosedPublic

Authored by tkrupa on Aug 9 2018, 2:38 AM.

Details

Summary

This was originally a part of D46179 but got so different to unsigned versions that it's better to have it in a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

tkrupa created this revision.Aug 9 2018, 2:38 AM
tkrupa edited the summary of this revision. (Show Details)Aug 9 2018, 2:39 AM

The title of this patch includes "Constant folding" - shouldn't the code be in ConstantFolding.cpp or some descendant of that?

I remember that there was a suggestion (years ago?) to split target-specific logic like this into its own files because other targets shouldn't be burdened with code that will never affect them, but this hasn't happened yet. There's x86-specific intrinsic code in ConstantFolding.cpp added with rL123206.

tkrupa added a comment.Aug 9 2018, 5:33 AM

I added it to InstCombineCalls.cpp after @craig.topper's suggestion to do so in order to enable adding more optimizations besides constant folding in the same place.

spatel added a subscriber: rnk.Aug 9 2018, 5:34 AM

I'm not finding the mails where the extraction of target-specific code came up, but my vague memory says @rnk had some ideas about how to improve things.

craig.topper added inline comments.Aug 9 2018, 10:04 AM
lib/Transforms/InstCombine/InstCombineCalls.cpp
265 ↗(On Diff #159884)

We should really remove masking from these intrinsics and use a select. But that can be a follow up.

286 ↗(On Diff #159884)

Probably could use getIntegerBitWidth instead of getPrimitiveSizeInBits. Should be slightly cheaper since getPrimitiveSizeInBits has to detect that its an integer and then call getIntegerBitWidth.

296 ↗(On Diff #159884)

const APInt & Val0

296 ↗(On Diff #159884)

Isn't it possible for the element to be a ConstantExpr? In which case this case would fail

tkrupa marked 3 inline comments as done.Aug 13 2018, 12:00 AM
tkrupa updated this revision to Diff 160295.Aug 13 2018, 12:04 AM

Implemented suggested changes.
Regarding masking with select - do you mean creating new avx512_padds/avx512_psubs intrinsics without masks and replacing the old calls with new intrinsic+select?

craig.topper accepted this revision.Aug 13 2018, 11:53 AM

LGTM with those two formatting fixes.

lib/Transforms/InstCombine/InstCombineCalls.cpp
288 ↗(On Diff #160295)

There's an extra space after 'unsigned'

305 ↗(On Diff #160295)

This is overindented.

This revision is now accepted and ready to land.Aug 13 2018, 11:53 AM
This revision was automatically updated to reflect the committed changes.