[PATCH] D84949: [JumpThreading] Don't limit the type of an operand

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:685
   // Handle Cast instructions.  Only see through Cast when the source operand is
   // PHI, Cmp, or Freeze to save the compilation time.
   if (CastInst *CI = dyn_cast<CastInst>(I)) {
----------------
lebedev.ri wrote:
> aqjune wrote:
> > lebedev.ri wrote:
> > > aqjune wrote:
> > > > efriedma wrote:
> > > > > Oh, hmm, I misintepreted this comment, and didn't read the code carefully enough.  The "if" is actually a heuristic that suppresses potential optimizations.
> > > > > 
> > > > > Do you have any idea what the actual compile-time impact would be if we just recursed over all casts?
> > > > I ran a test, and actually it brought a slight speedup when compiled with -O3 (without LTO):
> > > > ```
> > > > 
> > > > +-----------------------------------------------+-------+-------+------------+
> > > > |                   unit:sec.                   | base  | cast  | speedup(%) |
> > > > +-----------------------------------------------+-------+-------+------------+
> > > > | CTMark/7zip/7zip-benchmark.test               | 90.18 | 89.83 | 0.39%      |
> > > > | CTMark/Bullet/bullet.test                     | 63.11 | 62.92 | 0.31%      |
> > > > | CTMark/ClamAV/clamscan.test                   | 27.00 | 26.98 | 0.05%      |
> > > > | CTMark/SPASS/SPASS.test                       | 26.27 | 26.14 | 0.52%      |
> > > > | CTMark/consumer-typeset/consumer-typeset.test | 19.73 | 19.74 | -0.02%     |
> > > > | CTMark/kimwitu++/kc.test                      | 26.60 | 26.51 | 0.37%      |
> > > > | CTMark/lencod/lencod.test                     | 34.64 | 34.65 | -0.03%     |
> > > > | CTMark/mafft/pairlocalalign.test              | 16.24 | 16.24 | 0.00%      |
> > > > | CTMark/sqlite3/sqlite3.test                   | 24.61 | 24.68 | -0.27%     |
> > > > | CTMark/tramp3d-v4/tramp3d-v4.test             | 49.35 | 49.24 | 0.22%      |
> > > > +-----------------------------------------------+-------+-------+------------+
> > > > 
> > > > ```
> > > > 
> > > > Would it be a right direction if I remove this condition in a separate patch?
> > > Compile time != run time
> > The table depicts compile time.
> > 
> > Compilation becomes slightly faster (or almost equivalent, assuming that they are errors) if the conditions are removed, interestingly.
> http://llvm-compile-time-tracker.com/compare.php?from=03116a9f8c2fc98577e153083aaf9b6a701ab8f9&to=503a232ea5a630ddacde3ba01498776b61c4c8d4&stat=instructions
> So it doesn't look like the cost is too great
Sure, we can do it in a separate patch.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:706
+    // Unlike cast, don't limit the type of Source because it generates a
+    // sub-optimal code compared to the case when branch conditions are unfrozen
     ComputeValueKnownInPredecessorsImpl(Source, BB, Result, Preference,
----------------
Maybe tweak this comment, if we're going to drop the check for cmpinst.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84949