This is an archive of the discontinued LLVM Phabricator instance.

[x86] Extend the manual ISel of `add` and `sub` with both RMW memory operands and used flags to support matching immediate operands.
ClosedPublic

Authored by chandlerc on Aug 25 2017, 5:02 AM.

Details

Summary

This is a bit trickier than register operands, and we still want to fall
back on a register operands even for things that appear to be
"immediates" when they won't actually select into the operation's
immediate operand. This also requires us to handle things like selecting
sub vs. add to minimize the number of bits needed to represent the
immediate, and picking the shortest immediate encoding.

The end result seems very nice though, and we're now generating
optimal instruction sequences for these patterns IMO.

A follow-up patch will further expand this to other operations with RMW
memory operands. But handing add and sub are useful starting points
to flesh out the machinery and make sure interesting and complex cases
can be handled.

Depends on D37130.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Aug 25 2017, 5:02 AM
chandlerc updated this revision to Diff 112680.Aug 25 2017, 5:18 AM

Clean up some stale cruft in the test.

chandlerc updated this revision to Diff 112682.Aug 25 2017, 5:42 AM

Switch to a more verbose (more lines of code) pattern for selecting opcodes in
the ADD and SUB block. This will become more important when adding AND, OR, and
XOR which all want the same behavior.

Still, suggestions for better ways to handle this are welcome. This is kind of
a lesson in why tablegen patterns are better.

craig.topper added inline comments.Aug 25 2017, 9:41 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
2068 ↗(On Diff #112682)

Why did the fourth parameter get indented funny?

2144 ↗(On Diff #112682)

This isn't safe if the flag user wants the C flag is it?

chandlerc updated this revision to Diff 112766.Aug 25 2017, 5:35 PM

Update with minor formatting fixes and a major fix to correctly detect usage of
CF from an operation and suppress toggling between ADD and SUB in that case.

We could in theory *invert* the usage in the case of conditional branches and
such but it doesn't seem worth it.

chandlerc marked 2 inline comments as done.Aug 25 2017, 5:36 PM

Comments addressed, thanks!

craig.topper added inline comments.Aug 25 2017, 11:25 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
1941 ↗(On Diff #112766)

Should we just check the uses of ResNo(1)?

2241 ↗(On Diff #112766)

We don't need to truncate the immediate type/value do we?

craig.topper commandeered this revision.Sep 1 2017, 11:26 PM
craig.topper edited reviewers, added: chandlerc; removed: craig.topper.
craig.topper edited edge metadata.

Made a couple revisions. Commandeering so I can upload.

Don't truncate the APInt when creating the target constants. Normal isel doesn't do this. Keep the MemoryVT as the type for the target constants.

When looking for carry flag uses, only look at uses of result 1 of the ADD/SUB to find CopyToReg or other flag using instructions.

chandlerc commandeered this revision.Sep 7 2017, 12:02 AM
chandlerc edited reviewers, added: craig.topper; removed: chandlerc.
chandlerc updated this revision to Diff 114123.Sep 7 2017, 12:03 AM
chandlerc marked 2 inline comments as done.

Rebase, including Craig's fixes.

Rebased, fixes included. Anything else before I land this?

This revision is now accepted and ready to land.Sep 7 2017, 9:34 AM
This revision was automatically updated to reflect the committed changes.