This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Relax type restriction for store merge
ClosedPublic

Authored by niravd on Jun 23 2017, 1:17 PM.

Details

Summary

Allow stores of bitcastable types to be merged by peeking through BITCAST nodes and recasting stored values constant and vector extract nodes as necessary.

Event Timeline

niravd created this revision.Jun 23 2017, 1:17 PM
niravd updated this revision to Diff 104263.Jun 27 2017, 1:52 PM

Add missing check for typesize equality and explicitly avoid truncating stores.

niravd updated this revision to Diff 104693.Jun 29 2017, 11:17 AM

Resolve outstanding comments and rebase over latned extracted NFCI patch.

Ignore this last patch. It should have gone to another diff. Will overwrite with correct version presently.

RKSimon edited edge metadata.Jun 29 2017, 11:20 AM

This looks like the D34472 patch?

niravd updated this revision to Diff 104706.Jun 29 2017, 11:34 AM

overwrite accidental patch.

RKSimon added inline comments.Jun 29 2017, 11:45 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12372 ↗(On Diff #104706)

Val.getOpcode()

12798 ↗(On Diff #104706)

StVal.getOpcode()

niravd updated this revision to Diff 105491.Jul 6 2017, 11:21 AM
niravd edited the summary of this revision. (Show Details)

Simpify, add comments, and address nits discussed.

niravd updated this revision to Diff 105494.Jul 6 2017, 11:29 AM
niravd marked an inline comment as done.

Addressed missed nits.

niravd marked an inline comment as done.Jul 6 2017, 11:31 AM
RKSimon added inline comments.Jul 6 2017, 11:56 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12523 ↗(On Diff #105494)

It'd probably be better if you avoid the ternary inside the if().

if (MemVT.isInteger() && !MemVT.bitsEq(Other->getMemoryVT()))
   return false;
if (!MemVT.isInteger() && Other->getMemoryVT() != MemVT)
   return false;

or

if ((MemVT.isInteger() && !MemVT.bitsEq(Other->getMemoryVT())) ||
    (!MemVT.isInteger() && Other->getMemoryVT() != MemVT))
   return false;

That or pull it out as a bool variable in the line above.

12536 ↗(On Diff #105494)

Another ternary inside an if()

niravd updated this revision to Diff 105867.Jul 10 2017, 9:06 AM

extract ternary operators from conditionals

niravd marked 2 inline comments as done.Jul 10 2017, 9:07 AM
niravd edited the summary of this revision. (Show Details)Jul 17 2017, 7:20 AM
niravd updated this revision to Diff 107098.Jul 18 2017, 7:50 AM
niravd edited the summary of this revision. (Show Details)

Cleanup the mergestore helper function casting and add missing extract_subvector casting logic.

niravd updated this revision to Diff 108509.Jul 27 2017, 11:31 AM
niravd retitled this revision from [DAGCombine] Improve Store Merge logic to merge bitcast extracts. to [DAG] Improve Store Merge candidate logic.
niravd edited the summary of this revision. (Show Details)

Rework to regularize and simplify bitcasting logic.

niravd updated this revision to Diff 109261.Aug 1 2017, 7:00 PM
niravd retitled this revision from [DAG] Improve Store Merge candidate logic to [DAG] Relax type restriction for store merge.
niravd edited the summary of this revision. (Show Details)

Rebase past landed NFC commits.

RKSimon added inline comments.Aug 4 2017, 8:36 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
484 ↗(On Diff #109261)

A lot of these comment changes look like NFCs that can go in separately.

12476 ↗(On Diff #109261)

Please can you create a static helper that performs this? We have a lot of peek through bitcasts in this patch alone....

12478 ↗(On Diff #109261)

Val.getValueSizeInBits()

12514 ↗(On Diff #109261)

Merge these ifs() to reduce indentation?

if (MemVT != Val.getValueType() &&
    (Val.getOpcode() == ISD::EXTRACT_VECTOR_ELT ||
     Val.getOpcode() == ISD::EXTRACT_SUBVECTOR) {
niravd updated this revision to Diff 110266.Aug 8 2017, 1:11 PM

Pull out some NFC changes and use peekThroughBitcast function

niravd marked 4 inline comments as done.Aug 8 2017, 1:13 PM
RKSimon added inline comments.Aug 9 2017, 7:21 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12487 ↗(On Diff #110266)

Why isn't this CFP->getValueAPF().bitcastToAPInt().zextOrTrunc(8 * ElementSizeBytes) ?

12638 ↗(On Diff #110266)

Pull the ternary operator out of the if()

12665 ↗(On Diff #110266)

NFC change

niravd updated this revision to Diff 110592.Aug 10 2017, 9:08 AM

Excise NFC change and address comments.

niravd marked 3 inline comments as done.Aug 10 2017, 9:09 AM
RKSimon accepted this revision.Aug 10 2017, 12:37 PM

LGTM - thanks

This revision is now accepted and ready to land.Aug 10 2017, 12:37 PM
This revision was automatically updated to reflect the committed changes.