In D104662#2831363, @lebedev.ri wrote:

Do we need to check that GEP index size is big enough for this?

I don't believe so, but maybe I'm missing something?

Corrected patch.

p.s. This could have landed without review.

@nathanchance - I'd suggest reverting until @mkazantsev can investigate. He's offline for the next few hours due to time zones.

Reading through the code again, I think I agree with your analysis.

In D104503#2829510, @efriedma wrote:

There are two relevant questions:

1. Does SCEV do something reasonable with such an expression? I mean, you obviously can't expand it; there isn't any viable insertion point. More generally, I'm not sure it's meaningful; even if we drop the assertions, it's not clear what the result is supposed to represent. If we want to support "subtracting" two addrecs to do dependency analysis, we should probably add a dedicated method, which returns something that isn't a regular SCEV expression.
2. Why is SCEVAA trying to construct such an expression? AliasAnalysis::alias() doesn't make any sense if there isn't a dominance relationship between the two pointers.

On (1), I think that point has convinced me we need to not create such scevs and return could not compute instead. Will give it a bit more thought, but that's the way I'm currently leaning.

For the record, some of the test diffs are a bit concerning code quality wise, but I think we may need to accept them to achieve the goal. At least a couple of them have obvious possibilities for improvement in follow ups if needed, so I'm willing to have this go in. We may end up having to revert, fix a few things, then reapply though. We'll see.

I would suggest splitting this into two patches. The "ignore undef input in phi" is safe, and commonly done across the optimizer. The "branch on undef" is tricky. Just from a risk management perspective, I think it makes sense to split them into separate changes.

For an optional follow up change, I believe we can avoid checking the wrap flags for one operand, if C1 and C2 share the same sign.

Comment much improved, thanks!

Response explained the test case interaction. LGTM w/a tweaked comment.

Would be LGTM, but I'd like to understand the called out test change first. Everything else looks as expected.

LGTM

In D104482#2827677, @nikic wrote:

In D104482#2827398, @reames wrote:

I think I would prefer to see the information for each exit as opposed to removing it entirely. When understanding why unrolling happened in a particular way, having the detailed information is often very helpful. Not a strong preference, I'm willing to LGTM this if you really object.

I'm not particularly familiar with how optimization remarks work -- if we wanted to go the other way around, we need to be able to identify specific exits in some way. For debug output, we can just print the block label, but I assume that wouldn't be suitable for optimization remarks, as the block label wouldn't really be meaningful in that context, it's an IR detail. Do you know how one would go about that?

Hm, good point I was thinking in terms of debug output, and hadn't recognized the ORE complication.

This looks really close to good to go.

FYI, agree with all the stylistic comments from Eli. Waiting on perf numbers to see if this is worth cleaning up, or if we need a radically different approach.

Max is correct. I think I can handle that easily, but let me make sure and then repost.

I think I would prefer to see the information for each exit as opposed to removing it entirely. When understanding why unrolling happened in a particular way, having the detailed information is often very helpful. Not a strong preference, I'm willing to LGTM this if you really object.

LGTM. Nice cleanup.

LGTM, concerns addressed by responses. The bit about which exits we fold is particularly subtle. Might be worth adding a comment to the code.

LGTM

Took a shot at a minimal crippling of SCEV over in https://reviews.llvm.org/D104403. That isn't a patch ready to land, but it should let us collect some perf numbers on the impact of crippling SCEV's ability to reason about non-integral pointers.

In D104322#2822104, @lebedev.ri wrote:

In D104322#2822083, @nikic wrote:

As a drive-by note, it would be great if you could expand LangRef on non-integral pointers a bit. It made sense to me when it specified that you can't use ptrtoint on a non-integral pointer, but without that limitation, it's not really clear to me what the actual difference between a non-integral pointer and a normal one is. What transforms are you not allowed to perform on a non-integral pointer that you can perform on a normal one?

Yes, i would also like to see such documentation, especially if non-integral pointers
are going to be used as an "arbitrary" roadblock for SCEV changes. I would have posted
that in the review for the commit mentioned, but there was none, which also highlights
the problem around non-integral pointer status in llvm :)

On the documentation side, I'd love to, but I'm honestly not sure *how* to. There's two inter-related problems here. The first is the semantics of an inttoptr is highly dependent on the target for non-integral pointers. At the moment, it can basically only be used to implement non-inlined built-in routines in any practical way. The second issue is that our definition of the integral pointer types themselves appear to be in flux, and are very vague about certain key details. The result is I'm left unsure how to formally specify them. This is why I used the implementation defined wording I did.

The revised change looks a lot more reasonable. With some cleanup, I'd be willing to LGTM this. I'm much happier with the explicit focus on avoiding the construction of a subtract of pointer type.

JFYI, I don't object to this change, but I am hesitant about the direction you seem to be indicating. Please don't use the excuse of our handling of non-integral types being somewhat broken to further break them. I'll comment on future reviews if appropriate.

In D103656#2821141, @efriedma wrote:

Having those queries return unknown more often would be undesirable.

I agree.

Second, the getLosslessPtrToIntExpr routine seems to not match what you're describing. It appears to have a bunch of cases where it does not succeed. Examples include min/max expressions, and udivs. (Going by code structure here. If I'm missing something, let me know.) Are you claiming that this routine is guaranteed to succeed? The existing code and comments don't seem to make this obvious.

SCEVPtrToIntSinkingRewriter always succeeds; it inherits implementations for every kind of expression from SCEVRewriteVisitor. The explicit implementations of visitAddExpr and visitMulExpr are just there to preserve the flags. Not sure how you're reaching the conclusion that it fails for min/max expressions.

Reading back through, I see you are correct. The only bailout (e.g. return of SCEVCouldNotCompute) is the effective type size check. Can you update/add some comments to the function to that effect?

The goal stated of having getSCEV(V)->getType()->isPointerTy() == V->getType()->isPointerTy() seems reasonable to me. I'm not as sure about the baseof(V) == baseof(S) bit, but I tentatively accept that as it's not my main confusion.

Sorry for the silence. Partly busy, and partly having a hard time explaining something which seems obvious to me. Let me give this another try.

I want to chime in on the philosophy bits raised in the last few comments.

I agree w/Eli's comments.

Not familiar with this code.

I think there's a problem with this code, but I'm surprised the existing test didn't catch it. Have you run this with explicit dom tree and loop info validation enabled?

For those following along, the last comment from Sam convinced me to do something I've been considering for a while. In commit ac81cb7e, I removed the verifier rules which made it impossible to properly test the logic in rs4gc with non-integral pointers and went ahead and updated the relevant tests.

Max, I'd still prefer the singleton, but frankly, I don't care enough to keep discussing this. Land what we've got; we can refactor at some point in the future.

I don't really have an objection to this, but have you looked at the -stack-size-section support? I think you might be able to get the same information that way. If that doesn't work for some reason, just let me know and we can move forward with the review of this.

@Ayal If I'm understanding you even partially correctly, it sounds like you're raising a code quality issue. That is, we may generate a dead epilogue loop (e.g. with a condition statically known to result in the epilogue being untaken, but emitted as a condition) when we didn't need to. Is that correct?

Drive by thought, not intended to be blocking.

In D97982#2806598, @MaskRay wrote:

@lebedev.ri @reames Could you weigh in from performance perspective?

JFYI, I no longer have access to the infrastructure used to test previous patches in this area. You could consider asking @skatkov if needed.

Change appears to be wrong as written, discussing why before adjusting.

Several level of comments.

I agree with the request to hold until latent miscompile in underlying code is addressed. I will hold this patch until that happens, and then land without further review. Max, good suggestion on condition ordering, will address in landed patch.

The regression for C programs should be addressed in aaaeb4b16. Note that this requires code motion done in b6ee5f2b.

JFYI, C and C++ apparently have different rules here. For c++ your example function is mustprogress, for C only the loop is. (Based on a quick skim of checkIfLoopMustProgress clang's CodeGenFunction.h) I have no idea if the C rules could be more aggressive here or not, you'd be best taking that up with a clang developer.

Here is what I think an example of a bug in the current computeBECount. Overflow is complicated, so I might be wrong here. Sanity check me?

@nikic I'm increasingly thinking that the computeBECount logic is just wrong. I believe you can trip the overflow to zero case with a loop with explicit (correct) flags. There's some comments and a guard on negative steps which appear incorrect, and I'm guessing were an attempt to work around an existing bug.

LGTM w/one required change.

When reviewing your https://reviews.llvm.org/D103732 (in particular, the constants.ll test which had inttoptr constant expressions), I realized that this patch is necessary due to unreachable code. I went ahead and landed a slightly tweaked version of this change. I believe you should now be able to rebase your D103732 without needing to XFAIL a test.

In D103700#2802779, @stefanp wrote:

In D103700#2799532, @nemanjai wrote:

In D103700#2799340, @reames wrote:

In D103700#2799338, @nemanjai wrote:

We will be happy to test this on both our LE and BE systems. @stefanp do you have time to test this out or should I do it?

Seems like overkill for this one given the change is trivial and the test is target independent.

I'd save that for when I go to reapply the original patch that exposed this.

Oh, sorry. I meant to say that we'd test with this patch and the reverted one applied together.

I added both patches and I did a quick test on the Big Endian Power PC buildbot machine.
This patch does seem to fix the original issue. However, after adding the two patches I have 4 new LIT test failues:

LLVM :: Transforms/LoopVectorize/first-order-recurrence-complex.ll
LLVM :: Transforms/LoopVectorize/loop-form.ll
LLVM :: Transforms/LoopVectorize/multiple-exits-versioning.ll
LLVM :: Transforms/LoopVectorize/unroll_nonlatch.ll

The original patch would definitely need rebased over changed tests, so that's not surprising.

In D103316#2789027, @markus wrote:

Wrt assume operand bundles I am thinking that those can always simply be dropped.

I think you can simply ignore operand bundle forms. This is not on by default, and the implementation isn't yet mature. The folks working on that can pay the cost of figuring out what to do for them. As long as your code doesn't crash on operand bundle assumes, I don't really care what it does with them.

In D103316#2787008, @jdoerfert wrote:

In D103316#2786946, @nikic wrote:

This kind of transformation should probably eventually reside in lib/Transforms/Utils/SimplifyCFG.cpp but for now it seemed easier and a smaller change to just leave it in CodeGenPrepare.

I don't think this transform makes a lot of sense in CGP, I'd prefer it to go directly to SimplifyCFG (after which we can drop the CGP code).

I agree.

I think we would be better off to just drop these assumes. The framing I'm thinking of here is that SimplifyCFG should generally be ignoring ephemeral values in transforms. So if normally SimplifyCFG would drop a block that contains no instructions, then it should also drop a block that only contains ephemeral values.

I disagree.

if (x)
  assume(y)

is a natural way to spell something for the user.
We should not just drop it without having a good reason.
While we might not use assume(x && y) directly, we will use it if we specialize x to true in some places.

Er, I think you misread the comment you were replying to. The comment was discussing assume(A || B), your response is discussing assume(A && B). Given the context of the discussion, that difference seems important.

Can you take a step back and describe the problem you're trying to solve? I've read through the bug, but nothing there immediately makes me think "bug in core part of SCEV" as opposed to "bug in user of SCEV". How'd you get from there to here?

The implementation and description of this patch seem badly out of sync.

In D103492#2799986, @tolziplohu wrote:

I think RS4GC should make sure that the address space it uses has been specified non-integral and give an error if it hasn't, as this seems like an easy trap to fall into.

Good idea, post a patch and it'd be a quick LGTM.

LGTM still holds.

I went ahead and wrote up my thoughts on the optimizeable IR notion I mentioned upthread. You can read them here: https://github.com/preames/public-notes/blob/master/llvm-loop-opt-ideas.rst#should-scev-be-an-optimizeable-ir

LGTM

@nikic I found your last comment convincing.

In D103700#2799338, @nemanjai wrote:

We will be happy to test this on both our LE and BE systems. @stefanp do you have time to test this out or should I do it?

Seems like overkill for this one given the change is trivial and the test is target independent.

Fix silly typo in test.

Address reviewer suggestion.