This is an archive of the discontinued LLVM Phabricator instance.

[X86] Merge negated ISD::SUB nodes into X86ISD::SUB equivalent (PR40483) (WIP)
ClosedPublic

Authored by spatel on Mar 2 2019, 10:12 AM.

Details

Summary

Follow up to D58597, where I mentioned that the commuted ISD::SUB variant was having problems with lack of combines.

Any advice on how best to fix these regressions? Do we have a mechanism to propagate EFLAGS changes if we commute a X86ISD::SUB?

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Mar 2 2019, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2019, 10:12 AM

Instead of doing this combine by starting on the flag node. Could from the flag user and simultaneously rewrite the flag user and commute the flag node in the same combine?

If we rebase the tests after rL365711, I don't see any regressions. Not sure if we're getting all of the optimizations that were intended, but this patch seems safe to commit now.

xbolva00 added inline comments.
test/CodeGen/X86/jump_sign.ll
124

:(

spatel commandeered this revision.Jul 11 2019, 7:47 AM
spatel edited reviewers, added: RKSimon; removed: spatel.

Commandeering to post the rebased patch.

spatel updated this revision to Diff 209220.Jul 11 2019, 7:49 AM

Patch updated:
No code changes - just regenerated the test diffs.

xbolva00 accepted this revision.Jul 11 2019, 7:55 AM

This looks fine now

This revision is now accepted and ready to land.Jul 11 2019, 7:55 AM
lebedev.ri accepted this revision.Jul 11 2019, 7:59 AM
lebedev.ri added a subscriber: lebedev.ri.

Looks fine to me too.

RKSimon added inline comments.Jul 11 2019, 8:06 AM
llvm/test/CodeGen/X86/jump_sign.ll
307 ↗(On Diff #209220)

update comment?

spatel marked an inline comment as done.Jul 11 2019, 8:46 AM
spatel added inline comments.
llvm/test/CodeGen/X86/jump_sign.ll
307 ↗(On Diff #209220)

Yep - will update with commit.

This revision was automatically updated to reflect the committed changes.