This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Bugfix to avoid PartialStoreMerging involving non byte-sized stores
ClosedPublic

Authored by bjope on May 22 2019, 5:31 AM.

Details

Summary

The DeadStoreElimination pass now skips doing
PartialStoreMerging when stores overlap according to
OW_PartialEarlierWithFullLater and at least one of
the stores is having a store size that is different
from the size of the type being stored.

This solves problems seen in

https://bugs.llvm.org/show_bug.cgi?id=41949

for which we in the past could end up with
mis-compiles or assertions.

The content and location of the padding bits is not
formally described (or undefined) in the LangRef
at the moment. So the solution is chosen based on
that we cannot assume anything about the padding bits
when having a store that clobbers more memory than
indicated by the type of the value that is stored
(such as storing an i6 using an 8-bit store instruction).

Fixes: https://bugs.llvm.org/show_bug.cgi?id=41949

Event Timeline

bjope created this revision.May 22 2019, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2019, 5:31 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bjope updated this revision to Diff 200709.May 22 2019, 5:34 AM

Just a minor fix to use "Type *" instead of "auto *".

Makes sense

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1211

cast<> instead of dyn_cast<>

1215

If we don't already have a helper for "getTypeStoreSizeInBits == getTypeSizeInBits", we should add one to DataLayout.

bjope updated this revision to Diff 200826.May 22 2019, 3:10 PM

Add a helper in DataLayout "isTypeStoreSized()" as suggested in the review.
Open for suggestions regarding the name (or is it clear enough what it means?).

bjope marked 2 inline comments as done.May 22 2019, 3:18 PM
bjope added inline comments.
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1211

I no longer see a reason for calculating Earlier->getValueOperand()->getType() etc outside the if when using the new helper in DataLayout, so I reverted some of the refactoring in the last update. Also not sure if my earlier refactoring here was 100% safe.

efriedma added inline comments.May 22 2019, 4:02 PM
llvm/include/llvm/IR/DataLayout.h
460

The name you have is a little too short to be clear, I think; if you want something along those lines, maybe typeSizeEqualsStoreSize().

A couple other alternatives: hasNonStructPadding(), to indicate the underlying property of the memory. Or maybe follow EVT naming and call it isByteSized(), since this is essentially the same computation on a Type* instead of an EVT.

You could probably make this a little faster since getTypeStoreSizeInBits itself calls getTypeSizeInBits, but I guess it's unlikely to matter much.

bjope updated this revision to Diff 200979.May 23 2019, 8:13 AM

Now calling the new helper in DataLayout typeSizeEqualsStoreSize().

(If this gets accepted I'll follow up with an NFC patch that is using the new helper in various places.)

This revision is now accepted and ready to land.May 23 2019, 1:08 PM
This revision was automatically updated to reflect the committed changes.