Differential D61911
[GlobalOpt] Allow dead struct fields in SRA with non constant offset. Authored by chrib on May 14 2019, 10:36 AM.
Details
Allow struct fields SRA and dead stores. This works by considering fields accesses from getElementPtr to be considered as a possible pointer root that can be cleaned up. basically this allows the array in following C code to be optimized out struct Expr { int a[2]; int b; }; static struct Expr e; int foo (int i) e.b = 2; e.a[i] = 1; return e.b; }
Diff Detail
Event TimelineComment Actions Reverted in r361581. It regresses https://bugs.llvm.org/show_bug.cgi?id=38309 (represented by the testcase test/Transforms/GlobalOpt/globalsra-multigep.ll). Please be more careful in the future when you're changing an existing testcase, especially one with a comment that specifically says the transform you're trying to do is not valid. Comment Actions This fix the regression with multiple GEP arrays by allowing SRA for GEP ConstantExpr even if it might use an instruction containing indirect offsets access. The GlobalValue fields can be scalarized if they have StructType and not ArrayType with possible out-of-bound. Comment Actions Is this rebased? A fix landed in https://bugs.llvm.org/show_bug.cgi?id=38334 (last comment). Comment Actions I see. yes the fix is still there and I checked that this bug doesn't regresses. The fix for the GEP check from the last comment is still there, but moved it around the GlovalValue first level to be candidate for SRA . This allow StructType fields to be scalarized apart. as is 'a' and 'b' in struct Expr {
int a[3][3];
int b;
};
static struct Expr e;
int
foo(int i)
{
e.a[i][0] = 1;
e.b = 2;
return e.a[0][0];
}but multiple arrays non-constant accesses should be safe Comment Actions The problem here is that the IR semantics don't allow this transform in general. Even if you've managed to dodge the exact construct from bug 38309 (I'm not sure I understand how, but it's not really relevant), the rules for GEP indexing don't work the way you want them to. "inbounds" just means the result is somewhere within the same global; it doesn't mean that array indexes don't "overflow". See http://llvm.org/docs/LangRef.html#getelementptr-instruction . Comment Actions The desire here is to look at constant GEPs? Maybe looking at the "inrange" keyword instead would be better (it has stronger semantics than inbounds), "If the inrange keyword is present before any index, loading from or storing to any pointer derived from the getelementptr has undefined behavior if the load or store would access memory outside of the bounds of the element selected by the index marked as inrange. " (also discussed in http://llvm.org/docs/LangRef.html#getelementptr-instruction). Comment Actions Yes you are right, my idea was to avoid checking the non-cst index access for sub uses of the Value, in order to allow independent field GEP accesses to be candidate for SRA (for struct types(, but indeed just checking the first livel of ConstantExpr GEP might just be too complicated and I don't think that the inbound or inrange markers help here. (and multi-dimensional array GEP access would be inbound but wrong to SRA. What I'm thinking now, maybe, is to discriminate a valid SRA candidate with a common zero internalizer check for all the fields. so even an out-of-bound array access would not break the possible uses. Comment Actions the declaration from Bug #38309 currently generates @g_data = dso_local local_unnamed_addr global [8 x i16] [i16 1, i16 1, i16 1, i16 1, i16 0, i16 0, i16 0, i16 0], align 2 which seems better than a Struct LLVM IR construct as in : So if my assumption that the test globalsra-multigep.ll was badly constructed is wrong, there don't seem I can fix the semantic inconsistency with the C struct UB without inconsistencies with this regression test. sorry for the noise, abandoning this proposal |