@jrtc27 is correct. This absolutely must not apply to non-integral address spaces. It is not legal for LLVM to introduce additional ptrtoint instructions for non-integral address spaces that were not present in the original input IR. That doesn't change if the spelling of a ptrtoint/inttoptr pair is changed to bitcast. There is a bit of a larger issue here that LLVM IR isn't really rich enough to currently describe what operations are legal for the optimizer to introduce and what aren't. Every frontend/backend that uses them has their own rules that appear obvious for a particular use case, but aren't necessarily general. A similar issue applies to commuting GEPs with addrspacecasts. I'm wondering if we need something like the datalayout but for describing relationships between addrspaces and what things are legal and what are not.

So, I missed the original discussion of this, but I'm somewhat concerned about this direction. We very heavily rely on non-integral pointers in Julia. I'm concerned that removing the verifier rules will allow non-sound inttoptr/ptrtoint transformations to sneak in undetected. In general, I'm ok with the semantics proposed in this revision, but I would be much happier if they were disallowed entirely. Perhaps it is time to allow specifying more extensive address space attributes in the IR. For example, there is the constant discussion of whether geps may be commuted across addrspacecasts, which is just very end-user dependent. And now we have this possible distinction between "hard" and "soft" non-integral pointers. I think just letting frontends specify more precise semantics for their non-integral pointers may help alleviate them being pulled in so many different directions. Lastly, this may not be for the revision, but I take it the main motivation for this change is to be able to compute offsets between pointers to the same underlying object? I'm sympathetic to that use case, but could we just have a version of sub that did that directly? These offsets are well defined and stable, whereas allowing ptrtoint to give incompletely defined volatile answers seems very likely to wreak havoc.

Updated to add updates to test cases I'd forgotten to include in the revision.
They're entirely mechanical, but I'd appreciate a quick sanity check.

Verified to fix the crash we were seeing downstream as well.

I don't believe GlobalIsel is enabled. This is in debug mode, so
optimizations are enabled, but minimal. Thanks!

I believe this fix is incomplete and also needs to apply adrp/add/ldr sequences (and maybe others). We encountered this as a miscompilation while porting to Apple Silicon (https://github.com/JuliaLang/julia/issues/39820).

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.