Differential D63928
[ARM][SCEV][LSR] Prevent using undefined value in binops Authored by evgeny777 on Jun 28 2019, 6:07 AM.
Details This is an attempt to fix https://bugs.llvm.org/show_bug.cgi?id=42436 I find easiest way to fix the issue is to not creating undefined value when adding a cast in SCEV expander. For my sample function (see bug description)
Diff Detail
Event TimelineComment Actions Can you reduce your test case and making your check more specific? The description on the bugzilla ticket doesn't match up to your description and the CHECK in lsr-undef-in-binop.ll isn't helping me understand either. This comment was removed by evgeny777. Comment Actions Thanks for looking at it @samparker. I'll try to reduce the test case, but it's pretty hard to do this, because of lots of heuristics in LSR. I'll try to explain a bit further. The problem arises when SCEV expander tries to evaluate ((-1 * %7) + %9) (see lsr-undef-in-binop.ll) which should result to zero, but This results in broken IR after running opt -S -loop-reduce: %8 = ptrtoint i8* %7 to i32 ; ... %10 = ptrtoint i8* undef to i32 ; %10 is undefined ; ... .preheader: %120 = sub i32 %10, %8 ; the result of this binary operation is undefined %121 = sub i32 %120, %5 ; the result is undefined as well %scevgep8 = getelementptr i8, i8* %118, i32 %121 ; value of scevgep8 is undefined ; ... ; the loop below corresponds to construct_backwards called from std::vector and inlined 124: ; preds = %.preheader, %124 %lsr.iv = phi i32 [ 0, %.preheader ], [ %lsr.iv.next, %124 ] ; ..... loop body (skipped) %lsr.iv.next = add i32 %lsr.iv, -1 %tmp = inttoptr i32 %lsr.iv.next to i8* %126 = icmp eq i8* %scevgep8, %tmp ; The result of this comparison is undefined. On my device it runs until overwrites random memory location and program crashes br i1 %126, label %127, label %124 My patch attempts to fix the issue by not making cast instruction undefined. If you apply the patch you'll get: %8 = ptrtoint i8* %7 to i32 ; ... %10 = ptrtoint i8* %7 to i32 ; %10 is defined All values depending on %10 become defined. Comment Actions It looks like this problem is stemming from the fact that values are often cached during expansion, so even though the cast is replaced by a new value, the old value is still being used later on. Value reuse seems at important theme in this transform, so maybe we could try to introduce a map of updated values so that we don't reuse 'dead' instructions? I don't know what trade-offs we should be considering here though. This approach is certainly more simple and less likely to break again in the future. Comment Actions
I tried this initially but the problem is that previous result of expand could become undefined after subsequent call: Value *A = expand(Op1); Value *B = expand(Op2); // A may become undefined at this point There are lot of such potential "points of failure" in SCEVExpander and I don't easy way to address them all. Comment Actions Agreed that this is the simple and safe approach. It's worth adding a comment that this likely sub-optimal though. Comment Actions And please could you update the test so that is clear which instructions you are testing. Comment Actions
How? Is it ok to place comments in IR, like "this instruction below is converted to undefined value and bla bla bla ...."? Comment Actions There's a fair number of sub, load and ptrtoint instructions so narrowing the context of the checks would be good, just checking which basic block would be good. As well as what GEP the load is using. Thanks. |