GVN-hoist appears to miscompile SingleSource/Benchmarks/Misc/fbench.c as this builder: http://lab.llvm.org:8080/green/job/Performance_O3flto/ is broken after r277786 "GVN-hoist: enable by default". Citing Sebastians analysis: " think there is a problem with the MemorySSA when compiling fbench with -Ofast The problem appears with -mllvm -gvn-max-hoisted=63, (at 62 the program is not miscompiled.) before.ll is the MSSA dump at 62 and after.ll is the MSSA for 63. We end up hoisting two loads past a store, and the loads do not link to the store: they are both liveOnEntry: ; MemoryUse(liveOnEntry) %19 = load double, double* @axis_slope_angle, align 8, !tbaa !5 and ; MemoryUse(liveOnEntry) %22 = load double, double* @axis_slope_angle, align 8, !tbaa !5 past a store to the global variable: ; 7 = MemoryDef(liveOnEntry) store double 0.000000e+00, double* @axis_slope_angle, align 8, !tbaa !5 "
I reverted r277786 in the meantime (in r277909)
Created attachment 16895 [details] Preprocessed testcase Compile the attached file with: $ clang -Ofast fbench.i -fblocks -o a.out -lm -mllvm -gvn-max-hoisted=63 $ ./a.out [...] 93 errors in results. This is VERY SERIOUS. $ clang -Ofast fbench.i -fblocks -o a.out -lm -mllvm -gvn-max-hoisted=62 $ ./a.out [...] No errors in results.
Created attachment 16896 [details] MSSA with -gvn-max-hoisted=62
Created attachment 16897 [details] MSSA with -gvn-max-hoisted=63
Created attachment 16898 [details] Complete dump you can use to see incorrect version use opt -tbaa -print-memoryssa to see: if.then22: ; preds = %if.then20 ; 7 = MemoryDef(liveOnEntry) store double 0.000000e+00, double* @axis_slope_angle, align 8, !tbaa !3 ... ; 13 = MemoryPhi({if.then22,7},{if.else24,liveOnEntry}) ... ; MemoryUse(liveOnEntry) %22 = load double, double* @axis_slope_angle, align 8, !tbaa !3
Created attachment 16899 [details] Reduced version of testcase
This is a silly bug: We are trying to figure out whether the size of the stack has changed due to popping off non-dominating accesses,and we are not doing it right: We were doing: if (LocInfo.LowerBound >= VersionStack.size() - 1) { To see if the lower bound needed to be reset after popping the stack. The problem with this is that if you have any pushes, it may be that the lower bound is still wrong, the size will still now be "right". The correct fix is to store the size of the stack after we are done popping, and use it in the comparison above. Ironically, I had thought about this, and in the first versions of the use optimizer i sent george, we were storing the stack size, but i couldn't remember why we needed to do that so i removed it :) I'll fix this and add a testcase.
Created attachment 16900 [details] Patch that should fix it
Fixed in https://reviews.llvm.org/rL277978 and enabled gvn-hoist by default in https://reviews.llvm.org/rL278010