[PATCH] D152730: [ConstraintElim] Add A < B if A is an increasing phi for A != B.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 30 08:49:33 PDT 2023
fhahn marked 4 inline comments as done.
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1195
+ StepInst->getOperand(1) ==
+ ConstantInt::get(StepInst->getOperand(1)->getType(), 1)) {
+ StartValue = PN->getIncomingValue(1 - I);
----------------
nikic wrote:
> Does this check for step 1 still make sense, given the more generic checks below?
>
> Also, aren't we missing a check here that the GEP pointer operand is the PN? I think right now it can be unrelated...
> Does this check for step 1 still make sense, given the more generic checks below?
Not needed any longer, removed, thanks!
> Also, aren't we missing a check here that the GEP pointer operand is the PN? I think right now it can be unrelated...
Yep, added the check. The original test cases I had for that case didn't show the issue during verification, but added a couple of additional ones.
================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1213
+ StepConstantOffset) ||
+ !StepVariableOffsets.empty())
+ return;
----------------
nikic wrote:
> This looks like a complicated way to write accumulateConstantOffset().
Updated to use `accumulateConstantOffset`, thanks!
================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1218
+ auto *UpperGEP = dyn_cast<GetElementPtrInst>(B);
+ if (!UpperGEP || UpperGEP->getPointerOperand() != StartValue)
+ return;
----------------
nikic wrote:
> I think you need to require inbounds here as well. Otherwise, for non-power-of-two steps, you don't have a guarantee that the end value will be hit without wrapping (just that it is hit eventually).
Agreed, added the check. I don't think there's a way to add a test that triggers a miscompile because missing `inbounds` will prevent any further analysis of the GEP in the pass.
================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:1238
+ if (!UpperScale.urem(StepConstantOffset).isZero())
+ return;
+ }
----------------
nikic wrote:
> A cleaner (and more general) way to write this would be that all the scales/offset must be divisible by StepConstantOffset.
Yep, generalizing it also results in simpler code overall, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152730/new/
https://reviews.llvm.org/D152730
More information about the llvm-commits
mailing list