Details
- Reviewers
fhahn
Diff Detail
Event Timeline
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp | ||
---|---|---|
78 | Is the by-value pass here intentional? If so, should std::move be used in the initializer list? | |
187 | This is probably more accurately named IsKnownNonNegative. | |
231 | I'm wondering whether these should be sge and sle rather than sgt/slt. Edit: Hm, I guess MinSigned is excluded to avoid issues with various -1 multiplies? | |
243 | Hm, do we need an !IsSigned check here as well, as otherwise the base pointer + offset addition may overflow in a signed sense? | |
250 | Rather than checking for a specific GEP structure here, it might be more elegant to use accumulateConstantOffset() to handle any constant offset GEP? | |
268 | This precondition does not look correct. What if the inner GEP has more than one offset? What if it has a non-i8 element type? | |
307 | I think this condition can be incorrect if the index is truncating. For simplicity, maybe just reject indices of incorrect width? | |
315 | Ooops, this comment is outdated after recent changes. | |
399 | Are there any problems if Op1 is SIGNED_MIN here? | |
437 | Outdated comment. | |
507 | A bit odd that we only do this if the zero coefficients happen to be at the end. Edit: Ah, I guess we primarily care about determining whether there are no new variables at all here? | |
634 | I wonder whether this function is equivalent to DT.dominates(BasicBlockEdge(&BB, Succ), Succ)? | |
676 | Yeah, that would make more sense to me ... | |
689 | In this case, wouldn't a properlyDominates(&BB, Succ) check be sufficient? If we have something like assume; if() {A} B, then the assume holds in B. |
Thanks for taking a look! I pushed a couple of commits that addressed some points and also put up D139482.
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp | ||
---|---|---|
78 | Added std::move, but hopefully compilers should be able to generate good code after inlining even without it :) | |
187 | Renamed, thanks! | |
231 | Yeah, Val.sgt(MinSignedConstraintValue) ensures multiplying constants with -1 won't overflow. The upper bound could be sle. | |
243 | At the moment, this function isn't called for Signed, but I'll add an assert. I also added variants of the existing GEP tests but with signed predicates. | |
250 | I updated the code to use collectOffset here and instead of the loop over GEP indices below in | |
268 | Should be fixed in ee605b0accce by normalizing the offset. I tried various cases to see if I could construct a test case that fails verification without the condition, as inbounds GEPs shouldn't wrap around 0 when decrementing, hence we should be able to model this without the precondition I think. I'll plan to submit a patch for that separately. | |
307 | To my slight surprise, the implicit truncate preserves the signed value according to langref, so I think this should be fine? https://llvm.org/docs/LangRef.html#getelementptr-instruction
| |
315 | Updated in 6b940588a0fc, thanks! | |
399 | No, I think the only issue would be if we multiply SINGED_MIN constant with -1, but that's avoided above. If Op1 is SINGED_MIN here, it will be treated as variable. | |
437 | Will remove, thanks! | |
507 | Yep exactly, e.g. this is the case when one side adds and the other side subtracts the same value. | |
634 | Yep, updated, thanks! | |
676 | Updated in current main. | |
689 | This is not longer needed in the latest version after addressing the TODO above. |
Is the by-value pass here intentional? If so, should std::move be used in the initializer list?