This is an archive of the discontinued LLVM Phabricator instance.

[X86] Optimization for replacing LEA with MOV at frame index elimination time
ClosedPublic

Authored by zvi on Sep 18 2016, 1:26 AM.

Details

Summary

Replace a LEA instruction of the form 'lea (%esp), %ebx' --> 'mov %esp, %ebx'

MOV is preferable over LEA because usually there are more issue-slots available to execute MOVs than LEAs. Latest processors also support zero-latency MOVs.

Fixes pr29022.

Diff Detail

Repository
rL LLVM

Event Timeline

zvi updated this revision to Diff 71743.Sep 18 2016, 1:26 AM
zvi retitled this revision from to [X86] Optimization for replacing LEA with MOV at frame index elimination time.
zvi updated this object.
zvi added reviewers: mkuper, hfinkel, delena, igorb, myatsina.
zvi set the repository for this revision to rL LLVM.
mkuper edited edge metadata.Sep 19 2016, 11:54 AM

Thanks, Zvi!

lib/Target/X86/X86RegisterInfo.cpp
599 ↗(On Diff #71743)

This should be 'mov %esp, %ebx', without the parens.

606 ↗(On Diff #71743)

Are you absolutely sure all the operand that you getReg() here must be isReg()?

618 ↗(On Diff #71743)

I'm not sure this is correct.

If I understand the semantics correctly, LEA is special in that if the operand size is 64 bit, and the address size is 32-bit, it will only set the low dword bits of the source, and zero-extend. This will not happen if you do a 64-to-64-bit MOV.

628 ↗(On Diff #71743)

Have you made sure there are no iterator invalidation issues here? This gets called while iterating over the basic block.

zvi added a comment.Sep 21 2016, 12:05 AM

Thanks for the thorough review, Michael. I Will upload a revised patch,

lib/Target/X86/X86RegisterInfo.cpp
599 ↗(On Diff #71743)

Good catch!

606 ↗(On Diff #71743)

The use operands are defined as:
def lea64_32mem : Operand<i32> {
...

let MIOperandInfo = (ops GR64, i8imm, GR64_NOSP, i32imm, SEGMENT_REG);

...
}

618 ↗(On Diff #71743)

Yeah this incorrect. Thanks for catching that.

628 ↗(On Diff #71743)

Will check this.
I noticed that AArch64RegisterInfo::eliminateFrameIndex() calls rewriteAArch64FrameIndex() which calls MI.eraseFromParent() and for PPC there is a similar flow - not that this proves anything.

zvi updated this revision to Diff 72044.Sep 21 2016, 8:03 AM
zvi updated this object.
zvi edited edge metadata.

New revision addresses all Michael's comments except for the question whether erasing an instruction will invalidate an iterator - i am still looking into this.

zvi marked 6 inline comments as done.Sep 21 2016, 8:05 AM
mkuper accepted this revision.Sep 25 2016, 4:29 AM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 25 2016, 4:29 AM
This revision was automatically updated to reflect the committed changes.