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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:150
+  SmallSetVector<Value *, 64> OverdefinedInstWorkList;
+  SmallSetVector<Value *, 64> InstWorkList;
 
----------------
nikic wrote:
> Commit these changes separately?
Yes, I've removed them from the diff here. They are not required, just avoid doing some extra work. I'll submit those separately.


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


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1338
       mergeInValue(getStructValueState(&CB, i), &CB,
                    TrackedMultipleRetVals[std::make_pair(F, i)]);
   } else {
----------------
nikic wrote:
> Probably needs same handling as below?
Yes. I've added a test case for both the struct/non-struct versions here.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79036