Skip to content

Commit 3b79b2a

Browse files
committed
[SROA] Add an off-by-default *strict* inbounds check to SROA. I had SROA
implemented this way a long time ago and due to the overwhelming bugs that surfaced, moved to a much more relaxed variant. Richard Smith would like to understand the magnitude of this problem and it seems fairly harmless to keep some flag-controlled logic to get the extremely strict behavior here. I'll remove it if it doesn't prove useful. llvm-svn: 202193
1 parent 2230404 commit 3b79b2a

File tree

1 file changed

+42
-0
lines changed

1 file changed

+42
-0
lines changed

llvm/lib/Transforms/Scalar/SROA.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,11 @@ ForceSSAUpdater("force-ssa-updater", cl::init(false), cl::Hidden);
8585
static cl::opt<bool> SROARandomShuffleSlices("sroa-random-shuffle-slices",
8686
cl::init(false), cl::Hidden);
8787

88+
/// Hidden option to experiment with completely strict handling of inbounds
89+
/// GEPs.
90+
static cl::opt<bool> SROAStrictInbounds("sroa-strict-inbounds",
91+
cl::init(false), cl::Hidden);
92+
8893
namespace {
8994
/// \brief A custom IRBuilder inserter which prefixes all names if they are
9095
/// preserved.
@@ -392,6 +397,43 @@ class AllocaSlices::SliceBuilder : public PtrUseVisitor<SliceBuilder> {
392397
if (GEPI.use_empty())
393398
return markAsDead(GEPI);
394399

400+
if (SROAStrictInbounds && GEPI.isInBounds()) {
401+
// FIXME: This is a manually un-factored variant of the basic code inside
402+
// of GEPs with checking of the inbounds invariant specified in the
403+
// langref in a very strict sense. If we ever want to enable
404+
// SROAStrictInbounds, this code should be factored cleanly into
405+
// PtrUseVisitor, but it is easier to experiment with SROAStrictInbounds
406+
// by writing out the code here where we have tho underlying allocation
407+
// size readily available.
408+
APInt GEPOffset = Offset;
409+
for (gep_type_iterator GTI = gep_type_begin(GEPI),
410+
GTE = gep_type_end(GEPI);
411+
GTI != GTE; ++GTI) {
412+
ConstantInt *OpC = dyn_cast<ConstantInt>(GTI.getOperand());
413+
if (!OpC)
414+
break;
415+
416+
// Handle a struct index, which adds its field offset to the pointer.
417+
if (StructType *STy = dyn_cast<StructType>(*GTI)) {
418+
unsigned ElementIdx = OpC->getZExtValue();
419+
const StructLayout *SL = DL.getStructLayout(STy);
420+
GEPOffset +=
421+
APInt(Offset.getBitWidth(), SL->getElementOffset(ElementIdx));
422+
} else {
423+
// For array or vector indices, scale the index by the size of the type.
424+
APInt Index = OpC->getValue().sextOrTrunc(Offset.getBitWidth());
425+
GEPOffset += Index * APInt(Offset.getBitWidth(),
426+
DL.getTypeAllocSize(GTI.getIndexedType()));
427+
}
428+
429+
// If this index has computed an intermediate pointer which is not
430+
// inbounds, then the result of the GEP is a poison value and we can
431+
// delete it and all uses.
432+
if (GEPOffset.ugt(AllocSize))
433+
return markAsDead(GEPI);
434+
}
435+
}
436+
395437
return Base::visitGetElementPtrInst(GEPI);
396438
}
397439

0 commit comments

Comments
 (0)