[PATCH] D79036: [SCCP] Switch to widen at PHIs, stores and call edges.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks good from my side, but would be great if @efriedma can chime in as well.

One thing worth mentioning is that this change precludes D78376 <https://reviews.llvm.org/D78376>, at least for the loop case (I think for the non-loop case, it should automatically do the right thing as a result of this change). If widening is controlled at phis, that's what we'll set to overdefined. If we then limit one of the phi arguments to something better than overdefined based on the condition, we won't be able to make use of that anymore.



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:150
+  SmallSetVector<Value *, 64> OverdefinedInstWorkList;
+  SmallSetVector<Value *, 64> InstWorkList;
 
----------------
Commit these changes separately?


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1133
         mergeInValue(IV, &I, It->second,
-                     ValueLatticeElement::MergeOptions().setCheckWiden(false));
+                     ValueLatticeElement::MergeOptions().setMaxWidenSteps(10));
         return;
----------------
I'd suggest moving the `10` into a constant, as the value is repeated a few times.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1338
       mergeInValue(getStructValueState(&CB, i), &CB,
                    TrackedMultipleRetVals[std::make_pair(F, i)]);
   } else {
----------------
Probably needs same handling as below?


================
Comment at: llvm/test/Transforms/SCCP/widening.ll:877
+; CHECK:       bb.false:
+; CHECK-NEXT:    ret i32 [[A]]
+; SCCP-LABEL: @loop_with_multiple_euqal_incomings(
----------------
Some spurious check lines here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79036