Page MenuHomePhabricator

Address two correctness issues in LoadStoreVectorizer
ClosedPublic

Authored by asbirlea on Jul 1 2016, 12:48 PM.

Details

Summary

GetBoundryInstruction returns the last instruction as the instruction which follows or end(). Otherwise the last instruction in the boundry set is not being tested by isVectorizable().
Partially solve reordering of instructions. More extensive solution to follow.

Diff Detail

Event Timeline

asbirlea updated this revision to Diff 62526.Jul 1 2016, 12:48 PM
asbirlea retitled this revision from to Address two correctness issues in LoadStoreVectorizer.
asbirlea updated this object.
asbirlea added reviewers: llvm-commits, jlebar.
asbirlea added subscribers: arsenm, escha.
jlebar added inline comments.Jul 1 2016, 1:35 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
366

Can we have a comment here? Maybe just

// LastInstr is not inclusive.

Or alternatively maybe it would make more sense to leave this as-is and do

// Range is [first, last).
return std::maked_pair(FirstInstr, ++LastInstr.getIterator());

?

423

Nit, no need for "LSV:" -- the assertion error will point to this line of code. And, can we make this message a bit more specific? Like, clearly if an assert fails, there's an error. But what is the condition we're checking here, exactly? Maybe "Range [From, To) must contain all instructions in Chain"?

test/Transforms/LoadStoreVectorizer/AMDGPU/interleaved-mayalias-store.ll
5

Need a comma before "as" -- it's a conjunction separating two independent clauses.

test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll
2

Can we have a comment somewhere indicating what we're testing? Same for the other test. And maybe we should get rid of whatever instructions aren't relevant to the thing we're checking from our CHECKs?

arsenm added a comment.Jul 1 2016, 1:39 PM

You're missing the new lit.local.cfg for x86

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
197

This prints the entire block doesn't it? Maybe restrict to just the name? I think printing the entire block would be way too verbose for this, and you can already use the number to refer to the -print-before output

423

This message isn't descriptive of what is wrong and doesn't need the LSV

arsenm added inline comments.Jul 1 2016, 1:40 PM
test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll
2

Should this have -basicaa?

asbirlea updated this revision to Diff 62546.Jul 1 2016, 2:23 PM
asbirlea edited edge metadata.

Addressing comments.

jlebar added inline comments.Jul 1 2016, 2:32 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
423

Need to wrap to 80 chars.

But also this message can be tightened up, I think. No need to say "Error checking if chain is vectorizable:" -- we can tell from the stacktrace printed when the assert fails.

And as written this doesn't make sense to me:

All instructions in Chain should have been to ChainInsts from [From, To).

Maybe "to" is the wrong preposition? Or do you mean "should have been added to"? I still like my original suggestion, tbh. :)

asbirlea updated this revision to Diff 62547.Jul 1 2016, 2:37 PM
asbirlea marked 7 inline comments as done.

Shorten error message.

jlebar accepted this revision.Jul 1 2016, 2:38 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Jul 1 2016, 2:38 PM
asbirlea marked an inline comment as done.Jul 1 2016, 2:38 PM
asbirlea added inline comments.
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
197

This was a leftover, I removed it.

423

I meant "added to", realized that shortly after the update. Reupdated.

test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll
2

I don't think so. I can added it if you think this is the case.

asbirlea updated this revision to Diff 62548.Jul 1 2016, 2:42 PM
asbirlea marked an inline comment as done.
asbirlea edited edge metadata.

Adding lit.local.cfg. Renaming x86 to X86.

This revision was automatically updated to reflect the committed changes.