Skip to content

Commit 2d1b024

Browse files
author
Evgeniy Brevnov
committed
[DSE][NFC] Need to be carefull mixing signed and unsigned types
Currently in some places we use signed type to represent size of an access and put explicit casts from unsigned to signed. For example: int64_t EarlierSize = int64_t(Loc.Size.getValue()); Even though it doesn't loos bits (immidiatly) it may overflow and we end up with negative size. Potentially that cause later code to work incorrectly. A simple expample is a check that size is not negative. I think it would be safer and clearer if we use unsigned type for the size and handle it appropriately. Reviewed By: fhahn Differential Revision: https://reviews.llvm.org/D92648
1 parent 80766ec commit 2d1b024

File tree

1 file changed

+27
-13
lines changed

1 file changed

+27
-13
lines changed

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1071,8 +1071,8 @@ static bool handleEndBlock(BasicBlock &BB, AliasAnalysis *AA,
10711071
}
10721072

10731073
static bool tryToShorten(Instruction *EarlierWrite, int64_t &EarlierOffset,
1074-
int64_t &EarlierSize, int64_t LaterOffset,
1075-
int64_t LaterSize, bool IsOverwriteEnd) {
1074+
uint64_t &EarlierSize, int64_t LaterOffset,
1075+
uint64_t LaterSize, bool IsOverwriteEnd) {
10761076
// TODO: base this on the target vector size so that if the earlier
10771077
// store was too small to get vector writes anyway then its likely
10781078
// a good idea to shorten it
@@ -1127,16 +1127,23 @@ static bool tryToShorten(Instruction *EarlierWrite, int64_t &EarlierOffset,
11271127

11281128
static bool tryToShortenEnd(Instruction *EarlierWrite,
11291129
OverlapIntervalsTy &IntervalMap,
1130-
int64_t &EarlierStart, int64_t &EarlierSize) {
1130+
int64_t &EarlierStart, uint64_t &EarlierSize) {
11311131
if (IntervalMap.empty() || !isShortenableAtTheEnd(EarlierWrite))
11321132
return false;
11331133

11341134
OverlapIntervalsTy::iterator OII = --IntervalMap.end();
11351135
int64_t LaterStart = OII->second;
1136-
int64_t LaterSize = OII->first - LaterStart;
1136+
uint64_t LaterSize = OII->first - LaterStart;
11371137

1138-
if (LaterStart > EarlierStart && LaterStart < EarlierStart + EarlierSize &&
1139-
LaterStart + LaterSize >= EarlierStart + EarlierSize) {
1138+
assert(OII->first - LaterStart >= 0 && "Size expected to be positive");
1139+
1140+
if (LaterStart > EarlierStart &&
1141+
// Note: "LaterStart - EarlierStart" is known to be positive due to
1142+
// preceding check.
1143+
(uint64_t)(LaterStart - EarlierStart) < EarlierSize &&
1144+
// Note: "EarlierSize - (uint64_t)(LaterStart - EarlierStart)" is known to
1145+
// be non negative due to preceding checks.
1146+
LaterSize >= EarlierSize - (uint64_t)(LaterStart - EarlierStart)) {
11401147
if (tryToShorten(EarlierWrite, EarlierStart, EarlierSize, LaterStart,
11411148
LaterSize, true)) {
11421149
IntervalMap.erase(OII);
@@ -1148,16 +1155,23 @@ static bool tryToShortenEnd(Instruction *EarlierWrite,
11481155

11491156
static bool tryToShortenBegin(Instruction *EarlierWrite,
11501157
OverlapIntervalsTy &IntervalMap,
1151-
int64_t &EarlierStart, int64_t &EarlierSize) {
1158+
int64_t &EarlierStart, uint64_t &EarlierSize) {
11521159
if (IntervalMap.empty() || !isShortenableAtTheBeginning(EarlierWrite))
11531160
return false;
11541161

11551162
OverlapIntervalsTy::iterator OII = IntervalMap.begin();
11561163
int64_t LaterStart = OII->second;
1157-
int64_t LaterSize = OII->first - LaterStart;
1164+
uint64_t LaterSize = OII->first - LaterStart;
1165+
1166+
assert(OII->first - LaterStart >= 0 && "Size expected to be positive");
11581167

1159-
if (LaterStart <= EarlierStart && LaterStart + LaterSize > EarlierStart) {
1160-
assert(LaterStart + LaterSize < EarlierStart + EarlierSize &&
1168+
if (LaterStart <= EarlierStart &&
1169+
// Note: "EarlierStart - LaterStart" is known to be non negative due to
1170+
// preceding check.
1171+
LaterSize > (uint64_t)(EarlierStart - LaterStart)) {
1172+
// Note: "LaterSize - (uint64_t)(EarlierStart - LaterStart)" is known to be
1173+
// positive due to preceding checks.
1174+
assert(LaterSize - (uint64_t)(EarlierStart - LaterStart) < EarlierSize &&
11611175
"Should have been handled as OW_Complete");
11621176
if (tryToShorten(EarlierWrite, EarlierStart, EarlierSize, LaterStart,
11631177
LaterSize, false)) {
@@ -1179,7 +1193,7 @@ static bool removePartiallyOverlappedStores(const DataLayout &DL,
11791193

11801194
const Value *Ptr = Loc.Ptr->stripPointerCasts();
11811195
int64_t EarlierStart = 0;
1182-
int64_t EarlierSize = int64_t(Loc.Size.getValue());
1196+
uint64_t EarlierSize = Loc.Size.getValue();
11831197
GetPointerBaseWithConstantOffset(Ptr, EarlierStart, DL);
11841198
OverlapIntervalsTy &IntervalMap = OI.second;
11851199
Changed |=
@@ -1428,8 +1442,8 @@ static bool eliminateDeadStores(BasicBlock &BB, AliasAnalysis *AA,
14281442
"when partial-overwrite "
14291443
"tracking is enabled");
14301444
// The overwrite result is known, so these must be known, too.
1431-
int64_t EarlierSize = DepLoc.Size.getValue();
1432-
int64_t LaterSize = Loc.Size.getValue();
1445+
uint64_t EarlierSize = DepLoc.Size.getValue();
1446+
uint64_t LaterSize = Loc.Size.getValue();
14331447
bool IsOverwriteEnd = (OR == OW_End);
14341448
MadeChange |= tryToShorten(DepWrite, DepWriteOffset, EarlierSize,
14351449
InstWriteOffset, LaterSize, IsOverwriteEnd);

0 commit comments

Comments
 (0)