[PATCH] D115247: [EarlyCSE] Retain poison flags, if program is UB if poison.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 393563.
fhahn added a comment.

Updated again to be independet of D115527 <https://reviews.llvm.org/D115527> again, always use 'and' of flags for floating point insts.

In D115247#3185627 <https://reviews.llvm.org/D115247#3185627>, @spatel wrote:

> We have a few poison-related patches in progress currently, so there may be some interaction between these:
> D115460 <https://reviews.llvm.org/D115460> would add ninf/nnan to the list of poison-generating flags.
> D115526 <https://reviews.llvm.org/D115526> would remove a demanded elements restriction for propagation of flags

Thanks for those references. I think D115460 <https://reviews.llvm.org/D115460> would impact D115527 <https://reviews.llvm.org/D115527>, but not this patch directly. I went ahead and applied Philip's suggestion to always take the 'and' for floating point instructions.

In D115247#3185688 <https://reviews.llvm.org/D115247#3185688>, @reames wrote:

> Conceptually this is clear enough, but I'm worried about compile time impact.  Do you have any numbers to share?  This looks potentially quite expensive as the analysis to prove UB if poison is not really cacheable.
>
> We can improve compile time by e.g. only doing the search on things which might produce poison flags, but the first step is knowing if we have a problem.

Compile-time looks neutral when limiting the UB check to instructions with poison-generating flags, as in the original version, with the highest geoman increase of +0.04%. Note that SPASS for NewPM-ReleaseThinLTO has an increase of +0.17% and is highlighted red. For that workload, there's also a +0.16% size increase, so the increase is likely from additional transformations.

http://llvm-compile-time-tracker.com/compare.php?from=2a31b240df1ce1724960fd7cf98f673064b44206&to=e3d5cc5e3011f38eb95dc60f4a4762a40a7031a7&stat=instructions


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115247/new/

https://reviews.llvm.org/D115247

Files:
  llvm/lib/Transforms/Scalar/EarlyCSE.cpp
  llvm/test/Transforms/EarlyCSE/flags.ll


Index: llvm/test/Transforms/EarlyCSE/flags.ll
===================================================================
--- llvm/test/Transforms/EarlyCSE/flags.ll
+++ llvm/test/Transforms/EarlyCSE/flags.ll
@@ -22,7 +22,7 @@
 
 define void @test_inbounds_program_ub_if_first_gep_poison(i8* %ptr, i64 %n) {
 ; CHECK-LABEL: @test_inbounds_program_ub_if_first_gep_poison(
-; CHECK-NEXT:    [[ADD_PTR_1:%.*]] = getelementptr i8, i8* [[PTR:%.*]], i64 [[N:%.*]]
+; CHECK-NEXT:    [[ADD_PTR_1:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[N:%.*]]
 ; CHECK-NEXT:    call void @use.i8(i8* noundef [[ADD_PTR_1]])
 ; CHECK-NEXT:    call void @use.i8(i8* [[ADD_PTR_1]])
 ; CHECK-NEXT:    ret void
Index: llvm/lib/Transforms/Scalar/EarlyCSE.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/EarlyCSE.cpp
+++ llvm/lib/Transforms/Scalar/EarlyCSE.cpp
@@ -1366,8 +1366,17 @@
           LLVM_DEBUG(dbgs() << "Skipping due to debug counter\n");
           continue;
         }
-        if (auto *I = dyn_cast<Instruction>(V))
-          I->andIRFlags(&Inst);
+        if (auto *I = dyn_cast<Instruction>(V)) {
+          // If I being poison triggers UB, there is no need to drop those
+          // flags. Otherwise, only retain flags present on both I and Inst.
+          // TODO: Currently some fast-math flags are not treated as
+          // poison-generating even though they should. Until this is fixed,
+          // always retain flags present on both I and Inst for floating point
+          // instructions.
+          if (I->getType()->isFloatingPointTy() ||
+              !I->hasPoisonGeneratingFlags() || !programUndefinedIfPoison(I))
+            I->andIRFlags(&Inst);
+        }
         Inst.replaceAllUsesWith(V);
         salvageKnowledge(&Inst, &AC);
         removeMSSA(Inst);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115247.393563.patch
Type: text/x-patch
Size: 1839 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211210/992d493b/attachment.bin>