In D59722#1440346, @paquette wrote:

LGTM

Out of curiosity, do you have any numbers for how this impacts compile time on, say, CTMark? What percentage of an improvement can we expect from this?

Thanks Eli. In the case LastInstFound == BB->begin(), we have to renumber all
instructions after removing LastInstFound, so there is no need to manually number
the next instruction with 0.

Thanks Eli, I did not realize that there are quite efficient ways to compute the
expressions other than splitting it up and dealing with the even/uneven cases
with modulo.

For DSE, it seems quite straight-forward to preserve OrderedBB (we just remove instructions or replace existing ones with another one). I've added D59789 sketching that. This could be a stop-gap until this patch gets through.

I've bisected https://bugs.llvm.org/show_bug.cgi?id=41203 to this commit, which seems to cause crashes when building the test-suite with AXV512.

Ping. @hfinkel did my response make sense?

Ping. This applies the same solution as D59211 to a different case.

Thanks for all the feedback!

In D59156#1429460, @RKSimon wrote:

TBH I didn't expect that tests would be easy, but I don't think that should be a stopper to this patch (and code coverage indicates that all these cases are active).

Reduce number of dbg value calls to 4000.

In D59518#1434686, @probinson wrote:

Generating the test on the fly with Python seems plausible. Note you don't need 16K components, just enough components to make the location description exceed 16K bytes. If each component is (say) a 64-bit field with a max-int constant value, that's something like 11 bytes of description per field, so ~1500 fields ought to do it.

• Duh, not 16K bytes. 64K bytes. ~6000 fields.

Add test generated using Python and set size to 0.

On a first look, the patch looks useful! Any chance to provide the follow-up patch that uses it as well? that would be helpful to see the bigger picture. Also, are you seeing any benefits by preserving the state?

In D59156#1429460, @RKSimon wrote:

TBH I didn't expect that tests would be easy, but I don't think that should be a stopper to this patch (and code coverage indicates that all these cases are active).

In D59065#1429435, @hfinkel wrote:

I'm really afraid that this isn't sound. We've had a number of issues in this space, and we've always resisted attempts to make BasicAA look through inttoptr/ptrtotint. Once you convert to an integer, control dependencies can effectively add additional underlying objects. In cases where this is sound, why not fold away the whole inttoptr(and(ptrtoint)) in the first place?

Unfortunately this requires at least 64k stores to trigger AFAIK. It is triggered by the same input as D56740. I could try to reduce it, but I am not sure if such a big test case would be valuable?

ping

ping

Another case similar to D59211

Use wrapping arithmetic

use wrapping arithmetic

In D59211#1424945, @efriedma wrote:

It looks like LSR doesn't deal with this sort of issue consistently... some places we try to overflow-check arithmetic on the offset, and others we use wrapping arithmetic.

What should happen, really, is that each formula includes a bitwidth, and all arithmetic involving that formula use arithmetic that wraps at that bitwidth (like an APInt). Otherwise it's not really clear what the computation actually means.

Looks like at other places we cast one operand to uint64_t and then assign the result to int64_t. Given that the operations reflect formulas based on IR types, I guess that's valid? The LangRef does not explicitly mention the behavior of signed overflow for add & co, but given that it is using 2's complement, it should be defined, right?

Hi Dave, do you plan to commit this anytime soon?

In D59050#1420672, @efriedma wrote:

unordered_map is going to be pretty inefficient given the key/value types... it might be worthwhile to add some sort of DenseMap relative to LLVM that handles tombstones differently. (This also just came up in https://reviews.llvm.org/D59016.)

Use helpers from CheckedArithmetic.h for overflow checks, split out tombstone issue.

Just realized this went one step too far up the chain. The underlying object is the loaded value from the stack, but we cannot return that from GetUnderlyingObject, because it does not have a pointer type. I'll try to think of something else.

In D59009#1419379, @efriedma wrote:

Could you describe why you think this is correct? As far as I can tell, the "underlying object" you're returning isn't even related to the input pointer.

This partly addresses https://bugs.llvm.org/show_bug.cgi?id=40735

LGTM, given we already do the same for GenericScheduler. Please wait a bit with committing, in case there are additional concerns.

Thanks Eli! I've updated the message to say unrecognized, which sounds better to me too.

Thanks Sam! Looks like I copied the test without updating them....... Should be fixed now

Fix do_not_sink_nonfree_* tests to actually test what they are supposed to.

Sorry for the long delay....

Updated to catch more cases. This now fixes 4 fuzzer failures