This patch adds support more operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
While you're here, I'd suggest to go through all of Instructions.def and add an entry in the switch for all instructions that have a return value, so we at least know what we're missing. This is what I see:
- udiv, sdiv, urem, srem: If exact not set, recursively check operands. (Division by zero etc is undefined behavior, not poison.)
- shl, lshr, ashr: Apart from exact, these also have the bitwidth case, which could be checked for constant shamt.
- alloca: Never undef/poison. (The pointer, not the content...)
- load: We don't know. (Might want !noundef metadata in the future, if that's useful)
- atomicrmw, cmpxchg: No idea, no familiarity.
- trunc, sext, zext: Check operands, no special cases.
- fptoui, fptosi: These poison on overflow, can't infer anything useful.
- uitofp, sitofp: These stopped poisoning on overflow at some point, just check operands.
- fpext, fptrunc: Presumably just check operand, but I'm not familiar.
- ptrtoint, inttoptr: Check operand.
- addrspacecast: I would assume this is also just "check operand", but with addrspacecasts you never know :)
- call/invoke: Check noundef attribute on return value.
- select: Check operands and FMF.
- extractelement, insertelement, extractvalue, insertvalue: Check operands.
- shufflevalue: Have to take undef lanes into account.
It might make sense to add some of the straightforward cases (like trunc/sext/zext) to this review and leave the rest as TODOs.
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
4825–4826 | Should be cast<> rather than dyn_cast<>. | |
4827 | Move this case into the switch? Similar to how you handle FPMathOperator. | |
4828 | Use OpCheck(Opr->getOperand(0)) here? |
For this, canCreatePoison seems to be helpful. I'll add a patch that
- refactors canCreatePoison so it takes an operator
- add a function canCreateUndef
and make this patch invoke canCreateUndef.
Only one nit from my side, don't wait for me.
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
4827 | Nit: Can we have a comment /* something */ true so I know what true means ? |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
4791 | Added support for calls with noundef ret attribute since it's a small change |
llvm/include/llvm/Analysis/ValueTracking.h | ||
---|---|---|
611 | paddings -> padding | |
llvm/lib/Analysis/ValueTracking.cpp | ||
4795 | The positioning here is a bit odd, I'd move this below (or inside) the Operator check. | |
4836 | Does this conservative return still make sense? At this point this should only be an optimization (don't bother with dominator checks for const exprs) than something that is needed for correctness. |
No objections from me anymore.
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
4785 | Personally, I'd keep freeze up top, easy case after all ;). No strong opinion though. |
Since isGuaranteedNotToBeUndefOrPoison now traverses over many ops, there is 0.3% slowdown from ReleaseLTO-O3 in average on CTMark:
+-----------------------------------------------+---------+-------+----------+-------+-------+----------+ | | compile | | | link | | | +-----------------------------------------------+---------+-------+----------+-------+-------+----------+ | | base | | slowdown | base | | slowdown | | CTMark/7zip/7zip-benchmark.test | 89.31 | 89.96 | 0.72% | 46.73 | 46.93 | 0.43% | | CTMark/Bullet/bullet.test | 62.76 | 63.13 | 0.59% | 9.59 | 9.62 | 0.31% | | CTMark/ClamAV/clamscan.test | 26.91 | 26.95 | 0.13% | 24.70 | 24.82 | 0.48% | | CTMark/SPASS/SPASS.test | 26.07 | 26.18 | 0.42% | 19.60 | 19.72 | 0.60% | | CTMark/consumer-typeset/consumer-typeset.test | 19.58 | 19.65 | 0.36% | 18.41 | 18.49 | 0.44% | | CTMark/kimwitu++/kc.test | 26.50 | 26.56 | 0.22% | 14.54 | 14.51 | -0.21% | | CTMark/lencod/lencod.test | 34.48 | 34.47 | -0.03% | 38.38 | 38.67 | 0.76% | | CTMark/mafft/pairlocalalign.test | 16.17 | 16.25 | 0.44% | 8.64 | 8.64 | 0.00% | | CTMark/sqlite3/sqlite3.test | 24.49 | 24.42 | -0.28% | 24.43 | 24.52 | 0.36% | | CTMark/tramp3d-v4/tramp3d-v4.test | 49.14 | 49.10 | -0.08% | 35.64 | 35.85 | 0.59% | +-----------------------------------------------+---------+-------+----------+-------+-------+----------+
The solution for this doesn't seem trivial. Fixing existing undef/poison-related bugs might introduce more calls to this function, which will bring more slowdown.
What would be the right direction to address this issue?
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
4785 | Since constants and noundefs (after relevant patches are landed) are/will be very common, locating those checks at the top seemed beneficial. |
Unlike load/stores, the instructions we're targetting are likely to be changed by many transformations, maintaining a cache for the analysis result should be costly?
One possible approach that I can come up with is to reduce MaxDepth & add !noundef.
I'm not seeing any measurable impact from this change in terms of instructions retired: https://llvm-compile-time-tracker.com/compare.php?from=ef66e3d086308800d7947a385c2ae09d3f55a695&to=081c25553185740fde7d93d2a32ef4e87ae480aa&stat=instructions Most likely the difference you see is within noise for wall time measurements, which tends to be high.
Maybe it will become a problem in the future if this functions sees heavier usage, but I don't think compile-time is a big concern right now.
@aqjune Please can you take a look at https://bugs.llvm.org/show_bug.cgi?id=46831 (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24278) - it seems related to this patch
I also looked into this a day ago or so.
The problem seems to be that containsConstantExpression/containsUndefElement are called on non ConstantDataVector/ConstantVector, which segfaults. I will put up a patch soon.
@fhahn Yes, C was like <4 x i64*> getelementptr (i64, i64* null, <4 x i64> <i64 0, i64 0, i64 0, i64 -128>).
Thank you for the info..! I'm fine if you are making the patch.
clang-format not found in user's PATH; not linting file.