This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Assume 128bit multiply uses CTR
ClosedPublic

Authored by TimNN on Apr 6 2017, 3:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

TimNN created this revision.Apr 6 2017, 3:18 PM
hfinkel edited edge metadata.Apr 6 2017, 5:51 PM

A somewhat minified example IR which reproduces the assertion is attached to the linked bug report -- here I would also appreciate some guidance on in what form / where this can be integrated in the regression tests.

Just running llc on the resulting IR in the usual way should be fine.

lib/Target/PowerPC/PPCCTRLoops.cpp
250 ↗(On Diff #94445)

It looks like we can have a more-targeted fix than this. The problem seems to be restricted to multiply. See below.

317 ↗(On Diff #94445)

Here add:

case Intrinsic::umul_with_overflow: Opcode = ISD::UMULO; break;
case Intrinsic::smul_with_overflow: Opcode = ISD::SMULO; break;

and I think that should take care of it.

TimNN updated this revision to Diff 94534.Apr 7 2017, 9:39 AM
TimNN retitled this revision from [PowerPC] Assume intrinsics with 128bit integer operands use CTR to [PowerPC] Assume 128bit multiply uses CTR.
TimNN edited the summary of this revision. (Show Details)

Updated to a more targeted fix and added a test as suggested.

TimNN marked an inline comment as done.Apr 7 2017, 9:55 AM
TimNN marked an inline comment as done.
TimNN added a comment.Apr 10 2017, 9:36 AM

@hfinkel: Thanks for the feedback!

I have made the changes recommended and uploaded a new revision. (I though this would trigger another mail but apparently it didn't, so I'm commenting again).

hfinkel accepted this revision.Apr 10 2017, 11:49 AM

@hfinkel: Thanks for the feedback!

I have made the changes recommended and uploaded a new revision. (I though this would trigger another mail but apparently it didn't, so I'm commenting again).

Phabricator e-mails were broken for a while.

LGTM.

This revision is now accepted and ready to land.Apr 10 2017, 11:49 AM
TimNN added a comment.Apr 10 2017, 1:52 PM

I cannot commit this myself, could someone please do it for me?

This revision was automatically updated to reflect the committed changes.