[PATCH] D79299: [TRE][NFC] Refactor in preparation for removal of isDynamicConstant

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Please make the commit message describe what this patch is doing, as opposed on what you plan to do as a followup.  (It's okay to briefly mention the followup, but the commit should stand on its own for anyone reading it in the future.)

We usually prefer to put functions that are more than a couple lines outside the class.  The benefit is mostly just to reduce the indentation.



================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:448
 
-static CallInst *findTRECandidate(Instruction *TI,
-                                  bool CannotTailCallElimCallsMarkedTail,
-                                  const TargetTransformInfo *TTI) {
-  BasicBlock *BB = TI->getParent();
-  Function *F = BB->getParent();
+class TailRecursionEliminator {
+  Function &F;
----------------
Please make sure this class is in an anonymous namespace.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:457
+  bool TailCallsAreMarkedTail = false;
+  SmallVector<PHINode *, 8> ArgumentPHIs;
 
----------------
I'd prefer not to keep around mutable state like this in the class... but if you are, it needs better comments explaining what it represents and where it gets modified.

In particular, I was trying to work out what TailCallsAreMarkedTail represents, and ended up looking in circles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79299