This is an archive of the discontinued LLVM Phabricator instance.

[x86] improve CMOV codegen by pushing add into operands, part 3
ClosedPublic

Authored by spatel on Jul 27 2021, 2:30 PM.

Details

Summary

In this episode, we are trying to avoid an x86 micro-arch quirk where complex (3 operand) LEA potentially costs significantly more than simple LEA. So we simultaneously push and pull the math around the CMOV to balance the operations.

I looked at the debug spew during instruction selection and decided against trying a later DAGToDAG transform -- it seems very difficult to match if the trailing memops are already selected and managing the creation of extra instructions at that level is always tricky.

Diff Detail

Event Timeline

spatel created this revision.Jul 27 2021, 2:30 PM
spatel requested review of this revision.Jul 27 2021, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 2:30 PM

I think this makes sense.

llvm/test/CodeGen/X86/add-cmov.ll
277

This does seem like an improvement https://godbolt.org/z/obf4sGcKW

pengfei added inline comments.Jul 27 2021, 6:22 PM
llvm/test/CodeGen/X86/add-cmov.ll
284

lea won't affect eflags, so it may still be better than add in some complex scenarios I guess?

spatel added inline comments.Jul 27 2021, 6:37 PM
llvm/test/CodeGen/X86/add-cmov.ll
284

That's true in general, but I don't think it can be a factor in this transform because we know that the cmov is a user of eflags, so it is already being set by some other op.

RKSimon added inline comments.Jul 27 2021, 11:48 PM
llvm/test/CodeGen/X86/add-cmov.ll
284

We already have passes that convert add -> lea to break intefering eflags dependencies - I think this is fine.

RKSimon added inline comments.Jul 28 2021, 3:52 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
49972

We usually canonicalize add constants to RHS - does this actually cause problems?

spatel added inline comments.Jul 28 2021, 4:25 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
49972

I haven't been able to come up with a test to show this pattern, but I was worried that we could get in here in some intermediate non-canonical state.

If that happens, we'd likely hit an infinite loop with the previous transform, so I figured it was better to play it safe and check for constant.

RKSimon accepted this revision.Jul 28 2021, 5:14 AM

LGTM - cheers

This revision is now accepted and ready to land.Jul 28 2021, 5:14 AM