Page MenuHomePhabricator

DAGCombiner: Replace store of FP constant after attemping store merges

Authored by arsenm on Sep 8 2015, 9:35 AM.



If storing multiple FP constants, some subset of the stores
would be replaced with integers due to visit order, so
MergeConsecutiveStores would only partially merge these.

Alternatively, it possibly would be better to do this after LegalTypes

Diff Detail

Event Timeline

arsenm updated this revision to Diff 34224.Sep 8 2015, 9:35 AM
arsenm retitled this revision from to DAGCombiner: Replace store of FP constant after attemping store merges.
arsenm updated this object.
arsenm added a subscriber: llvm-commits.
spatel added a subscriber: spatel.Sep 15 2015, 11:23 AM

This appears to solve PR24654:

Can you split this patch into an NFC refactoring to add replaceStoreOfFPConstant() and then submit the logic changes as a separate patch? That would make it easier to read.

The AMDGPU FIXME test addition could be a separate commit too.

This appears to solve PR24654:

Matt - please can you add the x86/ppc test cases that are mentioned in PR24654?

arsenm updated this revision to Diff 35208.Sep 20 2015, 12:41 PM

Only do move in this patch. Add x86/ppc tests. I'll commit the refactor separately

spatel accepted this revision.Sep 21 2015, 7:21 AM
spatel added a reviewer: spatel.

LGTM - just a small comment on the test case settings.


We shouldn't need -mattr=altivec or darwin here; that was excess in the bug report.
Also, for this test and the next one: if you specify the triple, does it change anything to also specify the arch?

This revision is now accepted and ready to land.Sep 21 2015, 7:21 AM
arsenm added inline comments.Sep 21 2015, 8:41 AM

I don't think it does anything, but I usually specify both

arsenm closed this revision.Sep 21 2015, 9:01 AM