[PATCH] D137758: [HardwareLoops] remove loops which has undef count

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:447
+  // dedicated exit block, so we just delete these loops.
+  if (isa<UndefValue>(Count)) {
+    LLVM_DEBUG(dbgs() << " - Bailing, loop count is undef\n");
----------------
samparker wrote:
> nikic wrote:
> > shchenz wrote:
> > > samparker wrote:
> > > > shchenz wrote:
> > > > > fhahn wrote:
> > > > > > I doubt this is enough. I think you would at least have to ensure that the branch on undef is executed unconditionally in the loop. Also, shouldn't those dead loops be cleaned up earlier?
> > > > > > I think you would at least have to ensure that the branch on undef is executed unconditionally in the loop.
> > > > > 
> > > > > For hardware loops, the exiting block which contains the compare with loop count will be executed in each loop iteration.
> > > > > 
> > > > > > Also, shouldn't those dead loops be cleaned up earlier?
> > > > > 
> > > > > OPT has the pass that can clean up this kind of loop. But in LLC pipeline, there is no such pass.
> > > > Is there a blocker on adding the pass to the llc pipeline?
> > > hmm, to be honest, I didn't think about adding the pass to the llc pipeline. I am not sure if it is OK.
> > > 
> > > In OPT, for the case `remove-loop-count-undef-loop.ll`, the loops are optimized out by two passes, instcombine and loop deletion (Seems like loop deletion can only delete a simple loop optimized after instcombine)
> > > - `opt -loop-deletion  -S remove-loop-count-undef-loop.ll`, the loop is kept
> > > - `opt -instcombine -loop-deletion  -S remove-loop-count-undef-loop.ll`, the loop will be deleted.
> > > 
> > > I am thinking adding instcombine pass to llc pipeline is not a good idea and doing nested instruction combining to reduce loop complexity first in loop deletion pass also seems strange?
> > Generally speaking, if opt optimizes something, then llc should **not** optimize it, unless the optimization is only possible as a result of legalization. It doesn't sound like that's the case here?
> > 
> > The input to llc is assumed to be optimized IR -- if it isn't, it's sufficient to not crash or miscompile it, but there is no need to produce good code.
> Is adding the necessary pass(es) into the backend pipeline an option?
I think @nikic comment suggest llc should not be changed if the opt is able to do the expected optimization, unless the suboptimal IR is result of opt/llc.

I created this patch because I thought llc might be used standalone as llc itself is a functionality complete binary that consumes LLVM IR. But if that is not the case, I am ok to abandon this.

Thanks for all your comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137758