This is an archive of the discontinued LLVM Phabricator instance.

[x86] add cost overrides for mul with overflow
ClosedPublic

Authored by spatel on Oct 29 2020, 2:19 PM.

Details

Summary

I'm assuming the standard size integer instructions for this end up as something like:
mulq %rsi
seto %al

And the 'mul' generally has reciprocal throughput of 1 on typical implementations (higher latency, but that's not handled here).
The default costs may end up much higher than that, and that's what we see in the test diffs.

Diff Detail

Event Timeline

spatel created this revision.Oct 29 2020, 2:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 2:19 PM
spatel requested review of this revision.Oct 29 2020, 2:19 PM

Do we need to update the vector version cost as well?

Do we need to update the vector version cost as well?

Yes, but it will be more complicated because we will need to map operations/sizes to different ISA attributes. I think it is also less likely to see the vector versions of the intrinsics. For example, we can create this intrinsic from instcombine ( InstCombinerImpl::foldUnsignedMultiplicationOverflowCheck() ). But creating a vector version would mean that we had also encountered/created vector integer division.

My underlying motivation for this is that I am cleaning up the basic class cost model implementation of getIntrinsicInstrCost(), and if we don't have this x86 scalar override, there is a potential regression.
So for those reasons, I prefer to defer the vector implementation to follow-up patch(es). I can add a 'TODO' comment here if that is ok.

This looks reasonable to me FWIW, but i'm not very familiar with this area.

pengfei accepted this revision.Oct 30 2020, 5:20 AM

I see, thanks.
I agreed with implementing it in follow-up patches. And a TODO is good for it.
This patch looks good to me.

This revision is now accepted and ready to land.Oct 30 2020, 5:20 AM
This revision was automatically updated to reflect the committed changes.