- User Since
- Oct 23 2013, 6:32 PM (399 w, 5 d)
I don't believe so, but maybe I'm missing something?
p.s. This could have landed without review.
Reading through the code again, I think I agree with your analysis.
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!
Sat, Jun 19
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.
Fri, Jun 18
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.
Thu, Jun 17
Wed, Jun 16
LGTM, concerns addressed by responses. The bit about which exits we fold is particularly subtle. Might be worth adding a comment to the code.
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.
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.
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?
Tue, Jun 15
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?
Mon, Jun 14
Fri, Jun 11
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.
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.
Thu, Jun 10
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.
Wed, Jun 9
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.
Mon, Jun 7
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.
The original patch would definitely need rebased over changed tests, so that's not surprising.
Fri, Jun 4
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.
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.
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
@nikic I found your last comment convincing.
Seems like overkill for this one given the change is trivial and the test is target independent.
Fix silly typo in test.
Thu, Jun 3
Address reviewer suggestion.