[PATCH] D84949: [JumpThreading] Don't limit the type of an operand
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 3 13:48:14 PDT 2020
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
More information about the llvm-commits
mailing list