yaml2obj should allow you to create a broken object like that. IIRC there's an example in test/ELF.

So, from what I can see no transformation made for free() is valid in case of operator delete.
What do you think instead of introducing a predicate isCallToOperatorDelete() and bail out at the beginning of visitFree() ?
Alternatively, we could add a boolean argument to isFreeCall() which doesn't include operator delete (still trying to pick a name for the argument).

Side note: instcombine speculates these calls to free using pattern matching.
In my opinion, it shouldn't. Hoisting shouldn't be done in instcombine, and hoisting shouldn't be done on a per-name basis. Presumably, the frontend should attach an attribute to this call to make sure they can be hoisted.
If there's not enough information in the frontend, then the middle-end should compute this information (the attribute) and then some pass should be responsible for the sinking/hoisting.

r313372

I'm a little nervous about updating the dominator manually, still. I wonder if we can use the new API that Jakub implemented (cc: @kuhar / @dberlin )

LGTM

Thanks for fixing this.
I have no objections to the way you fixed it, some comments inline. Maybe Rui wants to take a look at the style.

In D37353#871147, @mssimpso wrote:

Added some unit tests. Thanks for the suggestion. I briefly considered moving SCCP over to the generic solver (as a simple way to re-purpose the existing tests), but this might not be that straightforward after all. The re-solving for undef values will need to be considered. In the end, adding unit tests for this patch wasn't that difficult to do.

In D37762#870202, @hfinkel wrote:

In D37762#870140, @rriddle wrote:

Has there been any sort of discussion on expanding/using the existing IR level code sinking pass? I am referring to the SinkingPass in scalar/sink.cpp. AFAIK it's only used in the AMDGPU preisel pipeline. I don't know it's current state/usability but the description of the pass is:

This pass moves instructions into successor blocks, when possible, so that
they aren't executed on paths where their results aren't needed.

Doing this as a late IR pass makes sense. Taking a quick look at the implementation, there may be some improvements that would help (e.g., make it use MemorySSA, better handling of debug info, use a postdom tree). Considering adding this to the default codegen IR pipeline could be a good idea (and maybe better than trying to do a better job at the MI level?).

I think what matters in this case is the {post}dominance relation between the block of the DEF and the block(s) [potentially > 1] of the USEs.
Doing this xform when all the uses are in the same block, is, correct, but restrictive. So, I think your logic is fine, but this makes me still less convinced that we shouldn't use the dom to drive this analysis (and therefore should be a separate pass :)

The changes to llvm-dwarfdump/llvm-objdump look good to me.
I didn't review all the tests, but that seems like something you did with grep, so I wouldn't be too worried if it passes the suite.

In D37353#867141, @mssimpso wrote:

In D37353#867090, @davide wrote:

If it's not too hard, you may consider adding some unit tests for the sparse engine.

Ah, I didn't think about adding tests to unittests/. That makes sense - I'll give it a shot. I do wonder if it'd be easier to just move SCCP over to the generic engine, though.

If it's not too hard, you may consider adding some unit tests for the sparse engine.

We should really move away from a model where we check in a binary for the dumper output.
Can you try to write a YAML file to generate the same object? (I think mc won't do it).

lgtm

Other than that, lg.

lg, thanks.

I don't think we want to do something fancier, at least for now.
If you run out of memory, we just report instead of crashing.
Presumably, if something shows up, we can have an API that propagates back errors to the caller which can take an appropriate action, but I don't see a compelling use case (maybe I miss one?)

I think this is on the right direction (for me), but let's see what other think.
If we agree, it will probably make sense revamping the general semilattice engine to plug arbitrary lattices in (and move the constant semilattice out).
This will enable future improvements (i.e. support for IPA bitwise CP, see, e.g. https://patchwork.ozlabs.org/patch/657644/)

In D24882#859796, @davide wrote:

I ended up in a situation where I needed 11 arguments (if you want a similar example, just grep for StringSwitch<bool> in MC).
I think the proposed solution was OK'ish up to 10 arguments, but adding more might be not ideal. So, now that we live in a post C++98 world, what do you think about switching this class to use variadic templating? I'm going to implement such a solution if nobody has objections. (cc: @dblaikie for thoughts)

I ended up in a situation where I needed 11 arguments (if you want a similar example, just grep for StringSwitch<bool> in MC).
I think the proposed solution was OK'ish up to 10 arguments, but adding more might be not ideal. So, now that we live in a post C++98 world, what do you think about switching this class to use variadic templating? I'm going to implement such a solution if nobody has objections. (cc: @dblaikie for thoughts)

I gave a round of light testing to this during the morning and it didn't expose any significant issue.
I agree with your plan, this looks like a good starting point. I'm not exactly looking forward to seeing fuzzer generated crazy miscompiles, so I hope this sticks together.

As Eli, I'd like to see other examples (I'm confident you can find some). In the meanwhile, a first review.

Sorry for being a little late to the party, but life took over recently :(
I don't mind the structure of the pass as is (and I saw @dberlin approved which increases my confidence).
I see this was reverted because it breaks the bot. Clement, I suppose you plan to recommit this soon, and after that happens, maybe @zhendongsu can enable the new cl::opt and find new trophies for the fuzzers we have available? Zhendong, what do you think?

All possibly addressed in r312015. Thanks for the comments, guys.

In D37153#854871, @mzolotukhin wrote:

I added a cl::opt...

You seemed to forget to use it in the test though :)

I would also give basic blocks and variables more meaningful names (specifically, I'd rename header and preheader blocks to show clearer, what happens in the test).

Michael

The issue was indeed that peeling was forced only on systemz.
I added a cl::opt and I was able to reproduce the bug on my machine (x86-64 Linux), and I was able to reduce the testcase to something much smaller:

I think I need a new cl::opt to force peeling count from loop unrolling.

In D37153#854336, @mzolotukhin wrote:

Hi,

The change looks good to me, but I'd like to see a more simplified test. We probably don't need 90% of its instructions, and also it doesn't need to be SystemZ-specific.

Michael

LGTM

Update Trip count/tripmultiple asking them to ScalarEvolution, as suggested by Eli.

FWIW, this is just an incarnation of a more general problem, i.e. the fact that the interaction between LTO and linker scripts is/wasn't well studied in the past.
I guess we may want to fix this case, but I'm afraid several other problems will arise.
I wonder when/where we should draw the line between supporting arbitrary interactions between the two or just suggesting people to pass -fno-lto to an handful of files in their link, as GCC seems to be doing.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65252
(also, cc @pcc for thoughts)

LGTM. And yes, I concur a bot is essential for fixing catching these early.

LGTM

Yes, I was afraid this wasn't enough.

What happens if foo is specified in the linker script but actually doesn't appear in any bitcode file?
IIRC we should error in that case.

Out of curiosity, where did you hit this?
Is there any way to test that the assignment updates the value of foo to be 1 ?

Independently from the style issues, this patch as is is a no-go.
Peter tried to extend this work and hit the N^2 pretty badly, so I'm not sure it makes sense to check-in it as is.

Address David's comments.

Indeed. I changed it & added a test while I was there.

I don't think we should drop DI in other cases (your reasoning is correct).

This is a decent cleanup. Feel free to submit without pre-commit review when it's obvious they're correct (and they're NFC).

Apologies, this should be fine.
Peter made me realize we should never see STB_LOCAL here.

Oh, fair enough, my bad.
I guess George's patch should go in instead. Let me abandon this.

Also, FWIW, we should consider making the same change to the gold plugin.

Personally, I'd still be interested in understanding whether this can be fixed in the consumers.
You probably want to hear what @rsmith thinks (i.e. whether it's feasible to fix this in clang, which seems to be the only place where this code would be used).
In case you can't, I'm OK with revisiting/allowing this feature in Support/.

Thanks for your review. I added a test for the other case and committed in r311351.