[PATCH] D33420: [LIR] Strengthen the check for recurrence variable in popcnt/CTLZ

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1042
+static bool checkRecurrence(Value *&VarX, Instruction *&DefX,
+                            BasicBlock *LoopEntry, PHINode *&PhiX) {
+  PhiX = dyn_cast<PHINode>(VarX);
----------------
evstupac wrote:
> davide wrote:
> > davide wrote:
> > > evstupac wrote:
> > > > I wonder if you need PhiX as a parameter. It looks like it is unused after the call.
> > > > Also it looks like the function does not modify anything, so you don't need references.
> > > Isn't it used in `recognizeAndInsertCTLZ` ?
> > Yes, I removed the refs.
> >Isn't it used in recognizeAndInsertCTLZ ?
> Yes. Right.
> I was confused by function name "check". Maybe we could calculate PhiX before the check (outside the function)?
> Or do like:
> 
> ```
> // checks that VarX and DefX create a loop recurrence and return corresponding phi node or NULL otherwise.
> static PHINode *checkRecurrence(Value *VarX, Instruction *DefX, BasicBlock *LoopEntry) {
>   PhiX = dyn_cast<PHINode>(VarX);
>   if (PhiX && PhiX->getParent() == LoopEntry &&
>       (PhiX->getOperand(0) == DefX || PhiX->getOperand(1) == DefX))
>     return PhiX;
>   return nullptr;
> }
> ```
I agree it should have a different name. I propose `getRecurrenceVar`


================
Comment at: test/Transforms/LoopIdiom/pr33114.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -loop-idiom %s | FileCheck %s
+
----------------
Updated, I'll change this before committing.


https://reviews.llvm.org/D33420