This is an archive of the discontinued LLVM Phabricator instance.

[ARM][AArch64][DAG] Reenable post-legalize store merge
ClosedPublic

Authored by niravd on Nov 30 2017, 9:08 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Nov 30 2017, 9:08 PM
eastig accepted this revision.Dec 1 2017, 2:00 AM

LGTM

This revision is now accepted and ready to land.Dec 1 2017, 2:00 AM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.Dec 1 2017, 11:51 AM
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12722

This isn't what a "truncating" store means for floating-point values. It converts the value using ISD::FP_ROUND. (We use nodes like this to model something like an x87 fstps; not sure if any other target uses it.)

eastig added inline comments.Dec 1 2017, 12:38 PM
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12722

Does it mean such stores should not be merged?

efriedma added inline comments.Dec 1 2017, 12:45 PM
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12722

We can still merge them... we just have to call APFloat::convert() to narrow the value to the right size, as opposed to an integer truncate. Or I guess we could just blacklist float truncstores.

eastig added inline comments.Dec 1 2017, 1:09 PM
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12722

Hmm, the original code was

StoreInt |= C->getValueAPF().bitcastToAPInt().zext(SizeInBits);

In r303802 Nirav changed it into the version on left. The commit comment is

[DAG] Prevent crashes when merging constant stores with high-bit set. NFC.

According to your comment truncating can destroy the value. So the commit was not NFC.

eastig added a comment.Dec 1 2017, 1:33 PM

Nirav,

There are stability and correctness issues on AArch64. The similar issues might exist on other ARM targets. Could you please disable MergeConsecutiveStores for all ARM targets including AArch64?

Thanks,
Evgeny

niravd reopened this revision.Dec 4 2017, 9:03 AM
This revision is now accepted and ready to land.Dec 4 2017, 9:03 AM
niravd added inline comments.Dec 4 2017, 9:55 AM
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12722

Eli:

Is that right?

Looking at LegalizeDAG, when we expand a TruncStore we do it as a store of a truncate which IIUC is strictly bitvector-level interpretation.

niravd updated this revision to Diff 125359.Dec 4 2017, 9:58 AM
niravd retitled this revision from [ARM][DAG] Reenable post-legalize store merge to [ARM][AArch64][DAG] Reenable post-legalize store merge.
niravd edited the summary of this revision. (Show Details)

I've disable store merges of FP constants with truncation as it's not clear what is the right behavior at the moment.

Evgeny, can you please test that this works for you?

niravd requested review of this revision.Dec 4 2017, 9:59 AM
niravd edited edge metadata.
efriedma edited edge metadata.Dec 4 2017, 12:49 PM

Looking at LegalizeDAG, when we expand a TruncStore we do it as a store of a truncate which IIUC is strictly bitvector-level interpretation.

Yes, the LegalizeDAG code will likely crash on a float TruncStore (but they don't show up outside of x86, so probably nobody noticed). But the intent is pretty clear if you look at how the x86 backend lowers them. There's also a comment in SelectionDAGNodes.h. But anyway, it's obscure enough that it probably doesn't matter if you disable the optimization.

eastig added a comment.Dec 4 2017, 1:39 PM

I've disable store merges of FP constants with truncation as it's not clear what is the right behavior at the moment.

Evgeny, can you please test that this works for you?

Sure, I'll test the patch tomorrow.

Thanks,
Evgeny

niravd added a comment.Dec 5 2017, 6:00 AM

Looking at LegalizeDAG, when we expand a TruncStore we do it as a store of a truncate which IIUC is strictly bitvector-level interpretation.

Yes, the LegalizeDAG code will likely crash on a float TruncStore (but they don't show up outside of x86, so probably nobody noticed). But the intent is pretty clear if you look at how the x86 backend lowers them. There's also a comment in SelectionDAGNodes.h. But anyway, it's obscure enough that it probably doesn't matter if you disable the optimization.

Weird. Post-legalized store for X86 has been turned on since early August. I'm surprised we didn't run into a bug sooner.

spatel added a subscriber: spatel.Dec 5 2017, 6:13 AM
eastig added a comment.Dec 5 2017, 1:30 PM

Hi Nirav,

This patch passed AArch64 NEON Emperor tests but it crashes on the test case reduced from spec2k6.

Thanks,
Evgeny

niravd added a comment.Dec 5 2017, 1:37 PM

I don't think I've seen that one. Can you send that test case to me so I can reproduce it?

eastig added a comment.Dec 5 2017, 1:43 PM

$  llc -O3 test_crash.ll
llc: /home/llvm-test/tmp/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1002: void (anonymous namespace)::SelectionDAGLegalize::LegalizeOp(llvm::SDNode *): Assertion `(TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) == TargetLowering::TypeLegal || TLI.isTypeLegal(Op.getValueType()) || Op.getOpcode() == ISD::TargetConstant || Op.getOpcode() == ISD::Register) && "Unexpected illegal type!"' failed.
...
niravd updated this revision to Diff 125723.Dec 6 2017, 6:51 AM

AArch64 failing case is failing because we did not check that constucted BUILD_VECTOR elements were legal.

Add test case and fix.

spatel added a comment.Dec 6 2017, 6:57 AM

For completeness/reference, I'm linking to D40790 which includes a failure test for SystemZ. It looks very similar to test/CodeGen/ARM/arm-storebytesmerge.ll, so I'm not sure if it adds value to have another target included in the tests when this is committed, but I thought I'd mention it here.

This revision was automatically updated to reflect the committed changes.