LGTM, but adding @mgorny and @hctim for D81921, which also tried to address the same issue as D44650 (which this replaces), in case I missed something that the former is addressing that this revision doesn't.

LGTM, from an IR legality perspective, but somebody familiar with GlobalISel should take a look from that perspective - adding authors/reviewers of https://reviews.llvm.org/D65167, which added this code. Also, we should have a test.

Well, the module if freed eventually. Is this issue whether the module if freed before the pass manager? If so I think run-twice will (incidentally also do that). I just though run-twice would catch it, because forgetting to clear such things usually causes incorrect behavior the next time around.

LGTM. Could we have a test for this? There is the -run-twice option to the various tools, which is designed to catch issues like this.

I think that would be fine, but in that case, we should probably get rid of x86andnp entirely and just make (and (not x) y) canonical everywhere. The supported patterns for these are much the same, so if we have both canonical forms, we're just signing up for double the work.

Should probably be an optional feature to the verifier pass, like FatalErrors?

Jinx ;) This took quite some tracking down on our side - too bad we didn't wait a couple of weeks. I do still like the iterator interface better, since it checks against exactly this kind of issue in debug mode. It also avoids the second lookup for the erase. I'll rebase this on top of master.

Turns out this doesn't quite yet address the TODO in Inliner.
Reverted that part of this change for a complete solution in a future
revision.

In D71156#1782580, @sbc100 wrote:

I have be working on CL that does basically a superset of this to support -gsplit-dwarf. But maybe can land this first.

LGTM. @lebedev.ri, do you have further review?

This broke the build for me locally:

In file included from /home/keno/llvm-project/llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp:10:0:
/home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h:36:29: error: declaration of ‘llvm::Optional<llvm::gsym::LineTable> llvm::gsym::FunctionInfo::LineTable’ [-fpermissive]
   llvm::Optional<LineTable> LineTable;
                             ^~~~~~~~~
In file included from /home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h:14:0,
                 from /home/keno/llvm-project/llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp:10:
/home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/LineTable.h:118:7: error: changes meaning of ‘LineTable’ from ‘class llvm::gsym::LineTable’ [-fpermissive]
 class LineTable {
       ^~~~~~~~~
[720/3364] Building CXX object lib/Analysis/CMakeFiles/LLVMAnalysis.dir/ScalarEvolution.cpp.o

Accidentally only pushed half the changes

Just out of curiosity, how did you discover this bug?

Ok, I think that'd work for my use case. I'll try it out and post a revision to that extent.

Hmm, we do have a list of all the .cpp files in the tablegen tool in ${ARGN}, and I'd be fine depending on the target libraries. Do you know of a way to emulate whatever handling add_executable is doing for .cpp files to get at the included .h files?

Is there a way to just add the whatever the underlying dependencies are that are missing for the native tool? My concern is that I don't want to build target executables (because the toolchain for that is broken).

Updated to add a test and confine the changes to the wasm backend code.

Use set_target_properties to set EXCLUDE_FROM_ALL on the target rather than using
the global setting.

There was a small omission in this revision that I fixed up in https://reviews.llvm.org/rG5bb0dcd96ec1.

In D63694#1558350, @sbc100 wrote:

Ideally I'd rather see the invalid file be generated on the fly, but this is also ok since it should never really change.

What about the actual test that checks for the error string? I imagine the binary would go in "Inputs" and the .test would be a text file that checks for the error.

Address review comments and add test case.

Remove extraneous output from error message and add test case.

This looks fine to me now. @reames if you have a chance, I'd appreciate confirmation that your concerns are addressed as well.

So, you'd like to make this a frontend flag in order not to expose it to "regular" end users? Or was it because, well, every other flag we have is a frontend flag?

Looked straightforward enough. Should be fixed by rPLO360238. I don't usually work on polly, so let me know if I misunderstood something.

@vtjnash Was the review comment addressed? If so I assume you want me to land this, since you don't have commit?

@sanjoy Could you take another look to make sure I have addressed your comments to your satisfaction before I commit this?

A few minor tweaks.

Rebase and address review comments.

In D61166#1481838, @sanjoy wrote:

and as I found out also stack overflows

For posterity, do you have some example IR and/or stack traces that you could attach to the commit message, or make a test case out of?

Shouldn't instead LLVMCFIVerify be declared using add_llvm_library with the proper dependency on the add_llvm_tool line. It seems like the problem might just be mixing the llvm and the cmake variant of these commands.

Actually, looks like the change to setLoopID is still relevant. I'll submit a new revision with just that change.

Looks like this got fixed by @jdoerfert in rL341926 with a near identical patch and test. No rebase required.

Alright, I have rebased this revision. I'd be happy to make the improvement to getAddExpr requested by @tvikram, since that seems independently useful, but I'd like to come to an agreement with @sanjoy and @mkazantsev on direction first.

Undo a small spurious change I noticed while browsing the diff

Rebased

Inline comment on implementation. The functionality looks fine to me.

Fix comment as requested by review, fix a small bug found in more extensive testing,
add a test case for the latter.

Fix ir names in tests broken by previous commits and proplerly match u/smin.

I have a general concern against such changes. In fact, you are introducing an alternative way to express the same thing. umin(a, b) and umax(~a, ~b) are the same, but now have 2 possible notations. It means that whatever pass or analysis that needs to recognize this pattern needs to be aware of both. Whenever the new node was not supported, it might be missed optimization opportunities. And we cannot know for sure how many such places there are, or will be. What motivation do you have for making this change? Is it strong enough to take a risk of missing optimization opportunities that I've just pointed out?

Fix a small bug discovered during production testing (
when expanding umin/umax, don't go to integers just because
types are unequal - the only reason to do is if one of the
operands in an integer and the other is not).

Add a small fix found during testing that I forgot to commit.

If my memory recovey is successful and I understand the problem correctly, the best solution to me is to separate the mutation and analysis code in StackProtector. Suppose we split it into two passes "StackProtectorAnalysis" and "InsertStackProtectorPrologue", will it solve the problem? During the re-run, we only run StackProtectorAnalysis, but never again insert prologue. StackProtectorAnalysis should be kept as-is, without more special logic to detect whether the prologue is already generated (as this patch does). What do you think?

This was merged in rL322311, closing to get it off my dashboard. In the future, ideally add

Differential Revision: https://reviews.llvm.org/D41960

with that syntax to the commit message, so Phabricator can close the revision automatically.

Remove incorrect comment from test case.