This is an archive of the discontinued LLVM Phabricator instance.

[X86] Limit Store Forwarding Block only to cases where we can prove that the memcpy does not overlap
AbandonedPublic

Authored by lsaba on Feb 22 2018, 6:49 AM.

Details

Summary

Limit Store Forwarding Block only to cases where we can prove that the memcpy does not overlap in memory, otherwise breaking the memcpy could result in overwriting the correct value.
This limits the optimization only to cases where AA can prove that the memory locations don't alias.

Diff Detail

Event Timeline

lsaba created this revision.Feb 22 2018, 6:49 AM

@chandlerc there's a good chance this solves the miscompilations you are seeing

craig.topper added inline comments.Feb 22 2018, 9:09 AM
lib/Target/X86/X86FixupSFB.cpp
465

Fix indentation of second parameter

503

Blank line between functions.

niravd added inline comments.Mar 21 2018, 9:12 AM
lib/Target/X86/X86FixupSFB.cpp
460

It looks like you're only using this to compare two operands so there's no reason to expose the internal reg/index value.

Pull the comparison into this function, e.g:

equalBaseOperands(MachineOperand &B1, MachineOperand &B2) {

if (B1.isReg() != B2.isReg()) return false;
if (B1.isReg())
    return B1.getReg() == B2.getReg();
return B1.getIndex() == B2.getIndex();

}

491

nit: The getParent check should probably be first.

495

Can MI/StoreMI have more than one memoperand? We should add an assert for a singleton list or check the crossproduct.

lsaba marked 5 inline comments as done.Mar 22 2018, 6:44 AM

Fixed comments in review https://reviews.llvm.org/D41330

zvi added inline comments.Apr 5 2018, 3:44 PM
lib/Target/X86/X86FixupSFB.cpp
64

Can you please commit the class rename change and rebase this patch on top?

460

Can this be committed and then rebase the patch on top?

lib/Target/X86/X86TargetMachine.cpp
65

Can you please commit the initialize-related changes and rebase?

test/CodeGen/X86/fixup-sfb.ll
7

Why were these cases removed?

lsaba abandoned this revision.Apr 6 2018, 1:31 AM

@zvi the changes in this commit were added to https://reviews.llvm.org/D41330 and committed already. I will close this patch