Page MenuHomePhabricator

[SelectionDAG][X86] Fix incorrect offset generated for VMASKMOV
ClosedPublic

Authored by aivchenk on Feb 8 2018, 1:23 PM.

Details

Summary

When creating high MachineMemOperand for MSTORE/MLOAD we supply
it with the original PointerInfo, while the pointer itself had been incremented.
The patch adds the proper offset to the PointerInfo.

Diff Detail

Event Timeline

aivchenk created this revision.Feb 8 2018, 1:23 PM

Are there MIR changes only? What changes do you expect in the asm code?

The problem originated from a miscompare in a benchmark. That appeared to be a machine-scheduler that moved writes after reads of a pointer. It turned out that the scheduler was within its rights to do so, as it checked the offset of the following pointer:

VMASKMOVPSYmr %stack.1.stack_output_vec, 1, $noreg, 32, $noreg, [[AVX_SET0_]], killed [[VMASKMOVPSYrm1]] :: (store 32 into %ir.stack_output_vec, align 4)

.. and it returned '0', instead of '32' I don't have a testcase-ready assembly at this point, but since the problem is local, I believe having MIR test to check that would be a better choice

delena added inline comments.
lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1215–1218

Why the Ptr should have only 2 operands? Can TLI increment memory address using GEP? Can it be done using ADD?
I don't think that you can assume anything about the TLI operation.

1219

The offset should always be the size of the Lo, right? Why 0? Why 32?

aivchenk updated this revision to Diff 134205.Feb 14 2018, 5:28 AM

That makes sense, thanks for the comments. In the new version there is a change in compress_expand.ll, which in my view is ok

no context in the patch

aivchenk updated this revision to Diff 134221.Feb 14 2018, 6:41 AM

Provided context

delena accepted this revision.Feb 14 2018, 7:22 AM
This revision is now accepted and ready to land.Feb 14 2018, 7:22 AM

@aivchenk Didn't you land this with rL325135? If you include 'Differential Revision: https://reviews.llvm.org/D43087' in your commit message it will close the phab for you.

aivchenk closed this revision.Feb 15 2018, 4:56 AM

Right, sorry about that. So the fix is committed as rL325135