This is an archive of the discontinued LLVM Phabricator instance.

[ConstraintElimination] Dummy revision for review
AbandonedPublic

Authored by nikic on Nov 11 2022, 6:58 AM.

Details

Reviewers
fhahn

Diff Detail

Event Timeline

nikic created this revision.Nov 11 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 6:58 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic requested review of this revision.Nov 11 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 6:58 AM
nikic added inline comments.Nov 15 2022, 8:10 AM
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.

nikic added a reviewer: fhahn.Nov 15 2022, 8:10 AM
fhahn added a comment.Dec 6 2022, 4:00 PM

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

ee605b0accce
6a834d2f2b96

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

If the type of an index is larger than the pointer index type, the truncation to the pointer index type preserves the signed value.

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.

fhahn added a comment.Dec 11 2022, 8:16 AM

I think with D137840 committed, the comments here should be addressed, thanks!

nikic abandoned this revision.Feb 22 2023, 7:40 AM

Never got around to reviewing the rest, and I doubt I will...