This is an archive of the discontinued LLVM Phabricator instance.

merge consecutive stores of extracted vector elements (PR21711)
ClosedPublic

Authored by spatel on Jan 5 2015, 2:58 PM.

Details

Summary

This is a 2nd try at the same optimization as http://reviews.llvm.org/D6698. That patch was checked in at r224611, but reverted at r225031 because it caused a failure.

The cause of the crash was not recognizing consecutive stores that have mixed source values (loads and vector element extracts), so I've just added a check to bail out if any store value is not coming from a vector element extract.

Diff Detail

Event Timeline

spatel updated this revision to Diff 17813.Jan 5 2015, 2:58 PM
spatel retitled this revision from to merge consecutive stores of extracted vector elements (PR21711).
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: samsonov, nadav, hfinkel.
spatel added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Jan 6 2015, 9:18 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10066

How does this cause a crash if you remove this check?

spatel added inline comments.Jan 6 2015, 6:46 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10066

For code like the second test case, we merge the stores and later during legalization get an error like this for the vector load:
Operand not processed?
0x5c4c770: v2i64,ch = load 0x5c4ac60, 0x5c4c660, 0x5c45850<LD16[%sunkaddr115](tbaa=<0x5b6c2a8>)> [ORD=61] [ID=1]

I haven't figured out exactly what happens to cause that.

hfinkel added inline comments.Jan 6 2015, 7:20 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10066

Is that load supposed to be dead? What did you do with users of the load's chain result (which I assume might include at least one of the stores you're merging).

10125

Maybe if the load appeared in the chain in between these stores somewhere, you'll cause a cycle?

spatel added inline comments.Jan 7 2015, 8:39 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10066

I didn't do anything with the load chain because I wasn't expecting any load-based sources in this code path. :)

Based on the later code (starting around line 9857) that handles sequences of loads, we need to replace the existing loads:

// Replace one of the loads with the new load.
LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[0].MemNode);
DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                              SDValue(NewLoad.getNode(), 1));

// Remove the rest of the load chains.

There are also a lot of other clauses down there that need to be met when replacing loads, so I was trying to handle only vector element extracts with this patch (this is similar to handling constant sources for the stores as in the clause above this one).

If it's ok with you, I can make this clearer in the comment here and then take a shot at the mixed source case later. I'm guessing the mixed store source case (some vector element extracts and some loads) isn't too common.

spatel updated this revision to Diff 18090.Jan 13 2015, 8:30 AM

Ping.

Updated comment to hopefully better explain the mixed source case that is not currently handled.

qcolombet edited edge metadata.Jan 21 2015, 10:35 AM

Hi Sanjay,

This looks almost good. Indeed, I believe we can refactor the code to avoid some code duplication. See my inline comment.

Thanks,
-Quentin

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10077

From this point, AFAICT, the code is exactly very similar to the constant version.
Please refractor the code with a helper method.

spatel updated this revision to Diff 18565.Jan 21 2015, 3:51 PM
spatel edited edge metadata.

Thanks, Quentin. Yes, there's a big block of copy/paste here. I've moved the duplicated logic into a helper function in this version of the patch.

qcolombet accepted this revision.Jan 21 2015, 6:02 PM
qcolombet edited edge metadata.

Hi Sanjay,

LGTM with some nitpicks.

No need for another round of review.

Thanks,
-Quentin

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
384

Use SmallVectorImpl<MemOpLink> instead.

9738

I would do one more refactoring here.
I would just check for UseVector and having an assert that if IsConstantSrc is true then UseVector must be true as well.

That way instead of having:
if (!IsConstantSrc) {

// look for type.
// loop.
// set res val

} else if (UseVector) {

// look for type.
// set res val.

} else {
}

We would have:
if (UseVector) {

// look for type.
if (!IsConstantSrc) {
  // loop.
  // set val.
} else {
 // set val.
}

} else {
}

The reasons why I suggest this (I am fine if you do not want to do that), are:

  1. The type creation will not be duplicated as well as the assert.
  2. At first glanced I found confusing to have !IsConstantSrc and UseVector ending in different blocks. Indeed, per the comment, if it is not a constant, this is a vector so having a vector not using vector is confusing.

The alternative would be to add a comment on the actual usage of UseVector in the function prototype.

This revision is now accepted and ready to land.Jan 21 2015, 6:02 PM
This revision was automatically updated to reflect the committed changes.

LGTM with some nitpicks.

Thanks, Quentin! I implemented your 2nd refactoring suggestion; I think that makes it easier to understand.