Ping!

Ping!

Ping!

Use an RAII object to always evaluate the arguments, except if HandleFunctionCall does it.

[No changes, just full context this time.]

If we're now catching integer overflow in more cases, please add some relevant testcases.

Rebase. Now that ObjCBoxedExpr change is in, we can remove Sema::CheckForIntOverflow entirely.

If the boxing method can't be constexpr then we can never evaluate it for a constant value, which means that we should unconditionally return Error, and use noteFailure to decide whether to visit the subexpr.

In D31839#724551, @ahatanak wrote:

OK, thanks for looking into it. Warnings for ObjCMessageExpr can probably be implemented in a separate patch.

It looks like clang still doesn't issue overflow warnings when the called functions have a void return. Should we try to fix it in this patch too?

void foo(int);

void test0() {
  foo(4068 * 1024 * 1024); // no warnings
}

In D31839#722763, @ahatanak wrote:

Is it possible to fix ObjCMessageExpr too while you are in here?

It looks like I LGTM'd it with comments that aren't resolved. If you don't have commit access, please upload a version which I can commit for you with no changes. Usually I LGTM+comments with the expectation that you can resolve the comments and commit without going through another round of review.

I'm concerned that the fix is too narrow. There's 10 callers to llvm::SplitEdge in LLVM and haven't audited them so I don't know whether they also check whether they also do this check. It's certainly not in the comment on SplitEdge that it's allowed to assert if you pass it a landing pad block.

I think the review this patch really needs is a comparison to GCC's implementation to double-check that it really does the same thing that GCC does. Is there a gcov user who could take a look at this?

Forgot clang changes!

For the IR level, I think this has got to be valid:

Closed by r272755.

Move the m_c_Op pattern matches out of ValueTracking.cpp and into PatternMatch.h. Reuse m_c_Or to simplify the new logic in InstructionSimplify.

Added testcase which I forgot to include the first time.

Just found this linked to from llvm weekly, and was wondering whether this works on the testcase in PR11331 and the dupes of that bug.

What about a ConstantExpr like "and(ptrtoint(@global), 32)"? We don't know the 5th bit of the address of @global, so C->isZeroValue() returns false (since we don't know it's zero), but isKnownNonZero must also return false (since we don't know it's non-zero). With your patch, we would incorrectly return true.

Spiffy!

(I'm not doing a full review, I just happened to notice a couple things when skimming.)

This is overlapping with D14003?

This adds a call to "processInvariantIntrinsic" in GVN.cpp, but there is no such function and it isn't defined in this patch?

I've been thinking about this patch and I don't understand what change you're trying to make at a high level.

Patch needs testcases.

Forgot to LGTM

I'm okay with either or both of changing existing testcases because of this change (ie., to represent the slightly weaker optimization) and/or adding a new testcase to ensure we don't get this exact thing wrong. I just didn't want it to go in with no change to the tests at all.

That is one big function.

LGTM

LGTM, but please do the obvious thing with zext too. I don't feel like the zext tests will need my review, but you can ask me to look at them if you feel otherwise. Thanks for the patch!

LGTM

The logic looks correct, and I really like the test!

I can't meaningfully review this, but I see nothing wrong here.

LGTM, thanks for the extensive gvn test!

LGTM with changes.

I just noticed that I'm listed as a reviewer here. I'm not familiar enough
with this are of llvm to review this patch. Do you need me to patch it in
and try it out?

A few unconnected issues:

LGTM

Is there a LangRef change that introduces the barrier? If that's simply in a different review, then LGTM. Usually I'd ask for the feature and the LangRef change to land together, but it also makes sense to put the LangRef update for !invariant.group and @llvm.invariant.group.barrier together.

LGTM

Forgot to write LGTM when I accepted revision.

Typo in summary, "There was ifinite loop" should say "infinite".

LGTM

This looks correct, but probably not enough.

LGTM

LGTM

This looks nearly ready to land. I've convinced myself that "MergePendingStores" is doing the right thing.

I'm worried about the memory usage of storing all of them. It feels like you traded CPU time for more memory when you didn't need to.