This patch enable loop reroll for the following case: for(int i=0; i<N; i += 2) { S += *a++; S += *a++; };
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi,
I've got a bunch of comments - I think it'll be easier to review if you address these first, then I can continue to the end of the patch.
James
lib/Transforms/Scalar/LoopRerollPass.cpp | ||
---|---|---|
430 | I don't see the need for the "Only" here. Simply "LoopControlIV" ? (no need for "ctrl" instead of "control" - be verbose when possible). | |
434 | The name should include "branch" - like "isCompareUsedByBranch" or something. It can also be made simpler: bool isCompareUsedByBranch(Instruction *I) { auto *TI = I->getParent()->getTerminator(); if (!isa<BranchInst>(TI) || !isa<CmpInst>(I)) return false; return I->hasOneUse() && TI->getOperand(0) == I; } | |
518 | Can't you just use InductionInfo::isInduction() here? You'd be looking for an induction with a step of 1 that is used by the loop latch. It seems this could remove a lot of code. | |
520 | Use "unsigned" here. There's no need to fix this to a specific bitwidth and no need for it to be signed. | |
521 | Please fold the ! inside the brackets for readability: if (IVUses != 2 && IVUses != 1) return false; | |
528 | IsCompInst | |
528 | isCompareInst doesn't handle nullptr. This should either use cast<Instruction>() or check that dyn_cast<Instruction>() is not nullptr. | |
531 | Same as line 531. | |
536 | You can fold these two if's together. | |
599 | No need for brackets: &*I. | |
1155 | This is really nasty. Please come up with a more concise way of expressing this. (If you used InductionDescriptor to represent LoopControlIV, this should be fairly easy I think?) |
Thanks, James.
Caught by tight release schedule again, will address your comment after freed up.
lib/Transforms/Scalar/LoopRerollPass.cpp | ||
---|---|---|
518 | Here I am checking if an induction PHI is only used to control loop iteration, no other uses such as A[i], normally, an Induction PHI will have other uses other than controlling loop iteration, e.g. Used as an index to access array. So InductionDescriptor::isInductionPHI is not enough here. | |
1155 | Here I am trying to mark the Loop Control Only IV and it's related instructions such as increment, compare and branches as used, InductionDescriptor can't help much here. And there seems no other way around it without adding more code, may be I could add more comments instead? |
Hi, James:
Patch updated, could you please continue your review?
Thanks a lot.
Lawrence Hu
Thanks James for the update, have a a nice vacation then.
Hal, could you please find some time to review it then? I have another reroll patch pending on this, and I will transfer to another team in May 21st.
Have a nice day.
Lawrence HU
lib/Transforms/Scalar/LoopRerollPass.cpp | ||
---|---|---|
167 | // For a loop with multiple induction variables, remember the one used only to control the loop. | |
513 | // Check if an IV is only used to control the loop. There are two cases: | |
514 | // 1. It has only one use which is the loop increment, and the increment is only used by the comparison and the PHI, and the comparison is used only by the branch. | |
516 | // 2. It is used only by the loop increment and the comparison, the loop increment is only used by the PHI, and the comparison is used only by the branch. | |
534 | be the loop increment | |
535 | The loop increment | |
544 | // The users of the IV must be a binary operation or a comparison. | |
1152 | // Make sure we mark loop-control-only PHIs as used in all iterations. See comment above LoopReroll::isLoopControlIV for more information. [Don't repeat the comment text from above LoopReroll::isLoopControlIV here.] | |
1170 | You don't need the parenthesis around UUser->user_begin() - '->' has higher precedence than '*'. | |
1171 | Do you need, or intend, the dyn_cast here? I think you don't, because you don't want the condition to be true if both sides are nullptr. | |
1435 | non-loop-control IVs | |
1451 | assert(COp && "Didn't find constant operand of LoopInc!"); | |
1455 | It does not look like there are test cases covering this logic, please add some. |
variable -> variables