This is an archive of the discontinued LLVM Phabricator instance.

[X86] Reuse EFLAGS and form LOCKed ops when only user is SETCC.
ClosedPublic

Authored by ab on Feb 25 2016, 6:23 PM.

Details

Summary

We only generate LOCKed versions of add/sub when the result is unused. It often happens that the result is used, but only by a comparison. We can optimize those out by reusing EFLAGS, which lets us use the proper instructions, instead of having to fallback to LXADD.

Instead of doing this as an MI peephole (as we do for the other non-LOCKed (really, non-MR) forms), do it in ISel. It becomes quite tricky later.

This also makes it possible to stop expanding and/or/xor if the only user is an icmp. I'm a little uncomfortable with the coupling, but I can send a patch if there are no objections?

Note that this builds on several local patches that I can commit right now if desired: I moved the LOCK isel from DAGToDAG to ISelLowering and tablegen. I added the atomic-eflags-update.ll file and split it out to make it easier to review; I'll squash it when committing.

Also, I locally added a combine to canonicalize (LSUB x, c) to (LADD x, -c), but only if c is 1 (to simplify matching INC/DEC). Any objections to generalizing that to any constant? (we do the same for ISD::SUB).

Diff Detail

Event Timeline

ab updated this revision to Diff 49132.Feb 25 2016, 6:23 PM
ab retitled this revision from to [X86] Reuse EFLAGS and form LOCKed ops when only user is SETCC..
ab updated this object.
ab added a reviewer: t.p.northover.
ab added subscribers: llvm-commits, jfb, qcolombet.
jfb added inline comments.Feb 26 2016, 9:49 AM
lib/Target/X86/X86ISelLowering.cpp
25538

I find the inverted condition hard to read: IIUC you're trying to continue if the opcode is CMP, or if the opcode is SUB and its result is unused? I know checkBoolTestSetCCCombine does the same, and it's equally confusing IMO :)

25571

Implementation for lowerAtomicArithWithLOCK?

ab updated this revision to Diff 49245.Feb 26 2016, 2:32 PM

Flip condition to make it more digestible.

ab updated this revision to Diff 49248.Feb 26 2016, 2:39 PM

Rebase away formatting changes.

ab added inline comments.Feb 26 2016, 2:47 PM
lib/Target/X86/X86ISelLowering.cpp
25538

Fair enough; flipped it and added a comment.

25571

It was in a local refactoring; uploaded D17659 for you!

jfb added a subscriber: jyknight.Mar 28 2016, 12:14 PM

I like this overall, but I'd be more comfortable if someone with better x86-fu that I also reviewed it.

lib/Target/X86/X86ISelLowering.cpp
25550

Note: related to @jyknight's D18201. Probably worth fixing (in a separate patch) once both land.

ab marked 4 inline comments as done.Apr 4 2016, 5:56 PM
ab added subscribers: spatel, RKSimon.

Thanks JF!

+more X86 folks; reviews appreciated.

qcolombet accepted this revision.Apr 5 2016, 9:33 AM
qcolombet added a reviewer: qcolombet.

Hi Ahmed,

LGTM.

Cheers,
-Quentin

This revision is now accepted and ready to land.Apr 5 2016, 9:33 AM
This revision was automatically updated to reflect the committed changes.