This patch folds more operations to poison.
Alive2 proof: https://alive2.llvm.org/ce/z/mxcb9G (it does not contain tests about div/rem because they fold to poison when raising UB)
Paths
| Differential D92270
[ConstantFold] Fold more operations to poison ClosedPublic Authored by aqjune on Nov 28 2020, 11:55 AM.
Details Summary This patch folds more operations to poison. Alive2 proof: https://alive2.llvm.org/ce/z/mxcb9G (it does not contain tests about div/rem because they fold to poison when raising UB)
Diff Detail
Event Timelineaqjune retitled this revision from [ConstProp] Fold more operations to poison to [ConstantFold] Fold more operations to poison.Nov 28 2020, 11:55 AM
Comment Actions update clang/test/Frontend/fixed_point_unary.c It seems unsigned _Fract type can only represent [0.0, 1.0)? I tried to find a relevant sentence from http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf , but couldn't. This revision is now accepted and ready to land.Nov 29 2020, 2:08 AM This revision was landed with ongoing or failed builds.Nov 29 2020, 4:20 AM Closed by commit rG53040a968dc2: [ConstantFold] Fold more operations to poison (authored by aqjune). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Hi, I'm seeing a miscompile with this patch. I'm not very good at the semantic differences between undef and poison so I don't know really where it goes wrong. Before Early CSE I see this in the input: entry: %cmp1 = icmp uge i16 1015, 16, !dbg !9 %shr = lshr i16 -1, 1015 %cmp2 = icmp ugt i16 2, %shr %or.cond = or i1 %cmp1, %cmp2, !dbg !10 br i1 %or.cond, label %if.then3, label %if.else4, !dbg !10 This corresponds to the following C-code: int16_t y1 = 1022; uint16_t res1 = 0; if (y1 < 0) res1 = 0; else res1 = 1015; if ((res1 >= 16) || (2 > (65535u >> res1))) res2 = 42; else res2 = 1; So in the C input, the right shift is guarded and won't be carried out if res1 is too large to avoid UB, but in the ll file the lshr is executed unconditionally. Then after Early CSE I instead get entry: br i1 poison, label %if.then3, label %if.else4, !dbg !9 So I suppose, with this patch the lshr will result in poison, and that poison will then make both %cmp2 and %or.cond be poison. And then we don't know where to jump so res2 gets the wrong value. I'm not very good at poison semantics, but it seems to me that either the input to ewarly cse is invalid, or the patch has broken the semantics of "or"? Comment Actions Hi, It seems it is related to two optimizations: Semantically, the first one is broken. It needs to freeze y. But, it will introduce a lot of freeze instructions. The clang patches that introduce noundef to function arguments is still ongoing. --- a/llvm/lib/IR/ConstantFold.cpp +++ b/llvm/lib/IR/ConstantFold.cpp @@ -1105,7 +1105,10 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1, } // Binary operations propagate poison. - if (isa<PoisonValue>(C1) || isa<PoisonValue>(C2)) + bool PoisonFold = !C1->getType()->isIntegerTy(1) || + (Opcode != Instruction::Or && Opcode != Instruction::And && + Opcode != Instruction::Xor); + if (PoisonFold && (isa<PoisonValue>(C1) || isa<PoisonValue>(C2))) return PoisonValue::get(C1->getType()); // Handle scalar UndefValue and scalable vector UndefValue. Fixed-length Comment Actions
Should langref also be updated to describe this? Or does it already? Comment Actions
I think "Values other than phi nodes and select instructions depend on their operands." was supposed to say that. :) Comment Actions
I see. Ok, thanks! Comment Actions We bisected a test failure to this (https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c17). Can you expand a bit on what this patch means in practice? I'm guessing it makes UB in C++ code have bad effects more often? If so, what type of UB? Comment Actions
This patch makes the result of erroneous operations in source to participate in constant folding in more eager manner. Comment Actions To fix the old bug quite a few patches in LLVM have landed so far and it is still ongoing. Comment Actions It turned out to be UB in our code as far as I can tell, see https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c36 and the follow-on. (If this is known to trigger an existing bug more often, it might still be a good idea to undo it until that existing bug is fixed? But we're not affected by this existing bug as far as I can tell, so this is just a suggestion.) Comment Actions
I don't know the policy of LLVM in this case, but it sounds legit. LLVM branching date is also near. @nikic May I ask your opinion? Comment Actions https://bugs.llvm.org/show_bug.cgi?id=49005 seems to be due to this (either directly or it has unearthed an existing problem) aqjune added a reverting change: rG06829034ca64: Revert "[ConstantFold] Fold more operations to poison".Feb 3 2021, 7:27 AM Comment Actions
I reverted this commit; I'll reland this after the underlying problem is resolved. MatzeB added inline comments.
neildhar added inline comments.
Comment Actions I bisected an arm 32 bit issue with WebKit Javascript's infinity value (https://github.com/llvm/llvm-project/issues/52669) down to this commit. Just to make people aware there is another failing case. No reproducer yet I'm still working on a way to reduce it, but it'll probably turn out to be covered by the points others have made. spatel mentioned this in D115804: [CodeGen] use saturating FP casts when compiling with "no-strict-float-cast-overflow".Dec 15 2021, 7:56 AM spatel added inline comments.
Revision Contents
Diff 308206 clang/test/Frontend/fixed_point_unary.c
llvm/lib/IR/ConstantFold.cpp
llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-fold-binop-select.ll
llvm/test/Transforms/InstCombine/apint-shift.ll
llvm/test/Transforms/InstCombine/canonicalize-ashr-shl-to-masking.ll
llvm/test/Transforms/InstCombine/canonicalize-lshr-shl-to-masking.ll
llvm/test/Transforms/InstCombine/canonicalize-shl-lshr-to-masking.ll
llvm/test/Transforms/InstCombine/icmp.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-a.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-b.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-c.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-e.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-variant-a.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-variant-b.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-variant-c.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-variant-d.ll
llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-variant-e.ll
llvm/test/Transforms/InstCombine/select-of-bittest.ll
llvm/test/Transforms/InstCombine/shift-add.ll
llvm/test/Transforms/InstSimplify/ConstProp/InsertElement.ll
llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
llvm/test/Transforms/InstSimplify/ConstProp/poison.ll
llvm/test/Transforms/InstSimplify/ConstProp/shift.ll
llvm/test/Transforms/InstSimplify/ConstProp/vector-undef-elts.ll
llvm/test/Transforms/InstSimplify/ConstProp/vscale.ll
llvm/test/Transforms/InstSimplify/div.ll
llvm/test/Transforms/InstSimplify/rem.ll
llvm/test/Transforms/InstSimplify/undef.ll
llvm/test/Transforms/SROA/phi-gep.ll
llvm/test/Transforms/SROA/select-gep.ll
llvm/test/Transforms/VectorCombine/X86/insert-binop-with-constant.ll
llvm/test/Transforms/VectorCombine/X86/insert-binop.ll
llvm/unittests/IR/ConstantsTest.cpp
|
I believe this is causing some of our clients trouble, especially since we somehow have a -fno-strict-float-cast-overflow flag in clang that says float->int overflows are not UB... (CC @spatel )