Seems like the more controversial preprocessor check in dirent was done in https://reviews.llvm.org/D119358, but we never got around to fixing the two header issues? We (Julia) are still carrying this patch downstream for just those two lines. @vchuravy can you rebase this with just those lines so we can close this?

Hmm, both semantics seems reasonable, and I don't think we can just make that decision for the frontend here. Perhaps at the IR level, we need a different syncscope property that declares whether it's expected to synchronize with non-temporal operations or not and then in clang we set it properly to match GCC (potentially with a matching __builtin_nontemporal_fence()?). I can see that frontends that make use of !nontemporal would expect fence to synchronize it.

I just looked at this again also, since this was requested in https://github.com/JuliaLang/julia/pull/48123 and came to the same conclusion, so LGTM, but it would be good to know from @craig.topper or @reames if there was any reason this wasn't done in D61863.

Updated to fix bug when lattice overapproximates a value

fixup CHECKs

clang-format patch

Remove reference C++ stdlib in comment

In D126424#3539242, @MaskRay wrote:

In D126424#3539240, @loladiro wrote:

In D126424#3539200, @MaskRay wrote:

#if __has_attribute(constructor) is unneeded. All supported GCC/Clang supports #if __has_attribute(constructor).

Ok, the comment seemed to indicate that this was being done for MSVC compatibility. Is building compiler-rt with MSVC no longer supported?

There is #define __has_attribute(attr) 0

In D126424#3539200, @MaskRay wrote:

#if __has_attribute(constructor) is unneeded. All supported GCC/Clang supports #if __has_attribute(constructor).

Updated to unconditionally use the init priority. Turns out the HAVE_INIT_PRIORITY config
check doesn't actually exist. However, for all our supported compilers that have the
constructor attribute, they should also have the constructor attribute with init priority,
which we use elsewhere in compiler-rt without extra check.

In D126424#3538733, @MaskRay wrote:

constructor(90) is more common.

Fix docstring typo

@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.