This is an archive of the discontinued LLVM Phabricator instance.

Don't reduce mul if the target doesn't support SSE2
ClosedPublic

Authored by wmi on Sep 6 2016, 6:26 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 70499.Sep 6 2016, 6:26 PM
wmi retitled this revision from to Don't reduce mul if the target doesn't support SSE2.
wmi updated this object.
wmi added reviewers: eli.friedman, mkuper.
wmi set the repository for this revision to rL LLVM.
wmi added a subscriber: llvm-commits.
emaste added a subscriber: dim.Sep 6 2016, 8:19 PM
emaste added a subscriber: emaste.
mkuper edited edge metadata.Sep 6 2016, 9:46 PM

Thanks, Wei.
The change LGTM, but can you fix the test up a bit?

test/CodeGen/X86/pr30298.ll
1 ↗(On Diff #70499)

I don't see why this test would require asserts. A copy/paste bug?

3 ↗(On Diff #70499)

Please add a positive check for the code we do expect.

wmi added inline comments.Sep 7 2016, 9:50 AM
test/CodeGen/X86/pr30298.ll
1 ↗(On Diff #70499)

It doesn't need it. I do want the test to be effective even if the test doesn't enable assertion. And I think that is why you suggest me to give a positive check. Thanks!

3 ↗(On Diff #70499)

Ok. will add it.

wmi updated this revision to Diff 70554.Sep 7 2016, 10:02 AM
wmi edited edge metadata.

Address Michael's comments.

mkuper accepted this revision.Sep 7 2016, 10:17 AM
mkuper edited edge metadata.

LGTM, except another small change to the test.
(No need to reupload the changed version for review, feel free to commit)

test/CodeGen/X86/pr30298.ll
6 ↗(On Diff #70554)

Can you mark the function nounwind, so that that we don't have all the cfi clutter?

This revision is now accepted and ready to land.Sep 7 2016, 10:17 AM
wmi added inline comments.Sep 7 2016, 10:39 AM
test/CodeGen/X86/pr30298.ll
6 ↗(On Diff #70554)

I was thinking how to remove the cfi directives but didn't a way to achieve it. Gcc option -fno-dwarf2-cfi-asm doesn't work for llvm.

Thanks a lot for the idea.

dim added a comment.Sep 7 2016, 10:42 AM

FWIW, for me it fixes both the original test case (from libx264), and the reduced test case from PR 30298. Thanks. :)

This revision was automatically updated to reflect the committed changes.