I am following, I've just been out for 3 weeks. I'm catching up now and this is in my queue.

Not a huge fan of boolean parameters like this, perhaps factor out the context as well into the caller and then we don't need it at all? Something else?

In D63306#1543925, @paquette wrote:

In D63306#1543020, @echristo wrote:

Haven't done much of a review, one drive by, but in general I like the idea. Would it make sense to have a general "remarks in object files" tool rather than just a size one? (It can, of course, only do size at the beginning.)

Thoughts?

Hmmm. That sounds similar to a more detailed version of opt-diff? (With added support for object files)

(I suppose that basically every pass can impact code size though, so I could see basically every remark being correlated with size.)

Do you have anything in particular in mind for what a more general tool would look like?

*points at pcc* :)

Haven't done much of a review, one drive by, but in general I like the idea. Would it make sense to have a general "remarks in object files" tool rather than just a size one? (It can, of course, only do size at the beginning.)

Mostly looks good, some inline comments and questions.

I'm tagging in Roland here since he and I were just discussing this the other day. I'm not entirely sure what we want to document here as I'm torn on whether or not we should be doing more here.

done thusly:

Some inline comment requests and if you wouldn't mind changing the type allocator separately it'd be great.

LGTM.

Be nice to split the predicate change from the short change. No need to re-review though, they're both fine.

In D61737#1497202, @jingham wrote:

I would rather not clutter up the lldb command driver's options with gdb command flags. That seems like it will make lldb harder to figure out and reduce our freedom to choose reasonable short names for lldb driver options.

It was reasonable to add lldb aliases for the gdb commands that you use tens to hundreds of times in a give debugging session - those get wired into your hands... But I don't think the same consideration holds for command line options...

If we feel the need to add a driver gdb compatibility mode like this, I like Rafael's suggestion of:

lldb --gdb <everything after this is handled by our gdb emulation parser>

This happened here:

Update comments. Have testcase work for multiple sizes and multiple preferred vector sizes.

Adding Sterling and Chris to this to take a look at the new layout :)

Arguably this could use some more comments and I'll add those as well.

LGTM. Sorry I didn't notice this earlier.

Committed thusly:

In D53379#1472539, @phosek wrote:

We have a use case for this other than Breakpad.

Sure. I'd have probably just checked for the function and maybe an instruction or two rather than the whole thing. It's more fragile that way and harder to understand what you're actually checking for since you mostly just said "doesn't crash" :)

Looks ok to me. Are you worried about the correctness of the code? That seems to be a lot of matching?

FWIW they're fine IMO too :)

OK, this code could use some work, but not for you to refactor it more than you already have.

This sounds like people are fine with me doing this...

LGTM.

In D60745#1468829, @hfinkel wrote:

Do we need to adjust the new pass manager too?

Update for the new pass manager as well. :)

In D60747#1469518, @echristo wrote:

In D60747#1469517, @george.burgess.iv wrote:

I can't find any users of this flag, so I'm in favor of s/EnableEarlyCSEMemSSA/true/ if that simplifies things.

Is this still useful for debugging problems with MemSSA and GVNHoist?

If we just have to tweak this one place, IMO it's sufficiently straightforward to just manually flip between true/false in code. I've personally never used it to debug anything, though I can only speak for myself.

That was pretty much my thought. As far as the new pass manager - the flag isn't being used anywhere else other than this location :)

In D60747#1469517, @george.burgess.iv wrote:

I can't find any users of this flag, so I'm in favor of s/EnableEarlyCSEMemSSA/true/ if that simplifies things.

Is this still useful for debugging problems with MemSSA and GVNHoist?

If we just have to tweak this one place, IMO it's sufficiently straightforward to just manually flip between true/false in code. I've personally never used it to debug anything, though I can only speak for myself.

In D60741#1467705, @dblaikie wrote:

Sounds good to me.
How'd you test that the MachineMemOperand.h changes were OK? Looks like it doesn't have an implementation file, so I'm not sure it's included first in any file - which means it's harder to tell if removing headers from it is valid/keeps it standalone (since its inclusion context might be allowing it to get away without including necessary headers)

Kristina has been in here the most, adding.

LGTM then.

Nick: Can you do some archaeology on the original patch and find out if there was supposed to be something supported here?