In D140726#4018463, @dblaikie wrote:

In D140726#4018459, @thakis wrote:

This is only used in lld cpp files, and lld's header files aren't meant to be used outside it. So halide is doing something unsupported.

I don't think Halide's doing something wrong here - it's including lld/Common/Driver.h which states:

// Generic entry point when using LLD as a library, safe for re-entry, supports
// crash recovery.

And that includes lld/Common/CommonLinkerContext.h which includes lld/Common/ErrorHandler.h

Could halide undef this macro after including the lld header and before including absl headers instead?

Perhaps? I guess alternatively lld/Common/Driver.h could undef it at the end, but including any other lld header would risk breakage (since they might expect the macro to still be defined) - would probably make implementing that header tricky - needing to redefine that macro back again in the implementation file, etc.

Not active really here, but wanted to point out --

In D112883#3101685, @GMNGeoffrey wrote:

Is this breaking actual users? I wouldn't expect it to break unless they depend on this target.

I don't know :-) I just know that people have sent me patches to remove the repository name dependence. I think https://github.com/llvm/llvm-project/blob/d1fdd745d510f40d8741d44ce39f5ae24ee7f91a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel#L1460 is the only previous instance in clang. That gets used only in "frontend". I'm not sure if there are people using "ast" but not "frontend"

Thanks, making suggested changes and then landing!

In D112883#3101645, @GMNGeoffrey wrote:

:-((((( Did you try the separate library with strip_include_prefix?

PTAL, and thanks for feedback so far!

Major update to better fix some of the issues here. No longer requires any changes outside of Bazel.

Sigh. The Bazel bits seem to break the automatic adding of lists. Ignore, I'll create a fresh revision with lists....

Updated, PTAL

Update based on review...

Add at least one obvious fix needed in Clang's build rules as well.

As mentioned in chat, there are other includes of uncopied files: https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/stdlib_stdexcept.cpp#L17

LGTM

LGTM

I don't think LLVM makes any attempt to be exception safe in this way, so not sure how important this is, but sure.

FWIW, I think this patch makes sense to me.

An alternative would be to delete this overload and insist callers pass a lambda that itself calls the desired function. This makes the user of function_ref more verbose, but it avoids having two dynamic dispatches (I think).

In D105338#2864865, @spatel wrote:

In D105338#2863990, @chandlerc wrote:

But just reverting this is pretty hostile unless Clang has been updated to make C and C++ volatile stores continue to behave as the language expects first.

Just regressing Clang because no one has written the LangRef change doesn't make sense to me. I think you should fix Clang to not be buggy first if you want to pursue this (or work with the Clang devs to get such a patch), just like we would fix a buggy LLVM pass that broke real users before enabling a valid optimization that uncovered the big.

I agree. I don't know exactly what fixes are needed for Clang, but we do know that code like this:
https://github.com/llvm/llvm-project/blob/8cf60e61e7b0e3213b5b81039878dcf1ef18c00f/compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp#L254
( the root cause of https://llvm.org/PR47480 ) is going to break again with this change.

If the goal is to get source like that to change, then we might as well fix that first, so we can point to it as a reference example of how to fix code?

In D105338#2863604, @jdoerfert wrote:

We actually hit this in the OpenMP device runtime. It seems nobody is interested in discussing a LangRef change. LGTM assuming there is no LangRef patch by Friday.

Doh, me too, sorry about that! LGTM!

LGTM, two minor suggestions below.

LGTM!

In D93169#2503960, @chandlerc wrote:

In D93169#2503947, @tianshilei1992 wrote:

In D93169#2503946, @chandlerc wrote:

I'm going to revert this as it breaks CMake on systems which do not have /proc/cpuinfo such as macOS.

This may be a bit hard to see because the code isn't reached unless the architecture is aarch64, but on an ARM macOS system that path hits. It would also hit on other BSDs or other OSes running on AArch64 but without /proc/cpuinfo.

For your reference, here is the error message from CMake for me:

CMake Error at /Users/chandlerc/src/llvm/llvm-project/openmp/runtime/cmake/LibompGetArchitecture.cmake:74 (file):
  file failed to open for reading (No such file or directory):

    /proc/cpuinfo
Call Stack (most recent call first):
  /Users/chandlerc/src/llvm/llvm-project/openmp/runtime/CMakeLists.txt:73 (libomp_is_aarch64_a64fx)

Will fix it right away.

I didn't think my CMake fu was up to it, but I think I have a fix, WDYT: https://reviews.llvm.org/D94889

Thanks for the fast review, landed in https://github.com/llvm/llvm-project/commit/f855751c1284c82c1c46b98f6d1b3ca2021d6cb9.

In D93169#2503947, @tianshilei1992 wrote:

In D93169#2503946, @chandlerc wrote:

I'm going to revert this as it breaks CMake on systems which do not have /proc/cpuinfo such as macOS.

This may be a bit hard to see because the code isn't reached unless the architecture is aarch64, but on an ARM macOS system that path hits. It would also hit on other BSDs or other OSes running on AArch64 but without /proc/cpuinfo.

For your reference, here is the error message from CMake for me:

CMake Error at /Users/chandlerc/src/llvm/llvm-project/openmp/runtime/cmake/LibompGetArchitecture.cmake:74 (file):
  file failed to open for reading (No such file or directory):

    /proc/cpuinfo
Call Stack (most recent call first):
  /Users/chandlerc/src/llvm/llvm-project/openmp/runtime/CMakeLists.txt:73 (libomp_is_aarch64_a64fx)

Will fix it right away.

I'm going to revert this as it breaks CMake on systems which do not have /proc/cpuinfo such as macOS.

In D80488#2341009, @vitalybuka wrote:

LGTM, but it could be better with a test

Update with a regression test.

Ping (and adding some sanitizer folks)? I'd really love to stop building with this local patch....

In D88788#2311004, @efriedma wrote:

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

In D88789#2310967, @efriedma wrote:

In D88789#2310606, @chandlerc wrote:

FWIW, I still very much feel that this is the correct canonicalization, and that downstream problems *must* be fixed downstream. Avoiding this canonicalization doesn't actually fix them, it just makes us less *aware* of the problems that still fundamentally exist. =[

I'd agree if we excluded all pointers from canonicalization. But the semantics of inttoptr and inttoptr-equivalent memory operations are weird; in general, I'm not sure we can recover the original semantics of the code if we throw away the pointer-ness of pointer load/store operations.

To address the issue at hand, I think changing the isNonIntegralPointerType() check to just isPtrOrPtrVectorTy() would be enough. I think that might make sense?

FWIW, I still very much feel that this is the correct canonicalization, and that downstream problems *must* be fixed downstream. Avoiding this canonicalization doesn't actually fix them, it just makes us less *aware* of the problems that still fundamentally exist. =[

Good to confirm with Dave of course, but this looks pretty great to me, and thanks so much!

FWIW, I would like to see *something* like this (both in the release and on trunk) but not sure this is actually the patch we want... Clang typically uses FIXME comments, and doesn't leave commented out code...

(Continuing the discussion here as I'm not really aware of a better place... feel free to point to a different thread if better... I don't actively follow llvm-dev at this point though, sorry if I've missed a post there.)

(And to be clear, IMO we should revert this patch while the discussion concludes as this is breaking real code.)

Regarding access to volatile memory...

In D87399#2263996, @nikic wrote:

however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

As already explained in the referenced revision, this is not correct. Volatile accesses are not permitted to trap. LangRef is unambiguous on this point:

The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation.

Somewhat minor post-commit thought, but we now have both a library FileCheck and an executable binary FileCheck (not to mention the existing confusion of having two FileCheck.cpp files).

Cool, thanks and LGTM!

LGTM!! Thanks for tracking down this surprising fix to the PR!

In D78471#2033046, @craig.topper wrote:

In D78471#2033001, @pengfei wrote:

In D78471#2032415, @craig.topper wrote:

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

Yes and no. It's the problem I wanted to fix firstly. But after I read the code of hardening, I think the spill is also a problem. Because that pass always load the function address to register at the beginning. I think the spilling violates the objective the pass does. Beside, according to calling conversation, there are volatile registers before calling. So I think it's not a cost if we pin the address register.

I think we should be calling NewMI->cloneInstrSymbols in the same spot we copy memory references in TargetInstrInfo::foldMemoryOperand. Or we shouldn't disable folding if any symbols are present. We're effectively cloning an instruction when we fold the memory and we should make sure we don't drop things when we do that.

LGTM, but one of these I think should be addressed here.

LGTM other than two nits here, this is really awesome!

As much as I'm not thrilled about the duplication, the number of comments I have below about the exact O1 pipeline sure make it seem valuable. Unsure how you feel about addressing these here, later, etc.

Wooot about finally having a test case! (Sorry for nit picking it a bit ....)

LGTM with the addition of skipping debug info as Eli suggests.

Might also be good to explain a bit of *how* this fixes the PR to the commit log (or bug) for posterity. It seemed to surprise both of us that this was the fix when talking through the code, I suspect a future reader may benefit from having a log of what went on here.

Really like the approach now. Pretty minor code nits below only. =D

Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test flow that ensures the pass builder DTRT?

Just wanted to say thanks for the performance data! I know it was hard to get, but it is really, really useful to help folks evaluate these kinds of changes with actual data around the options available.

I just want to mention here, that my comment on the original patch still stands and really needs to be addressed:

I'm seeing lots of updates to fix bugs, but no movement for many days on both my meta comments and (in some ways more importantly) James's meta comments. (And thanks Philip for chiming in too!)

In D68267#1733661, @jonpa wrote:

• CodeGen/X86/copy-eflags.ll

"X86 EFLAGS copy lowering" transforms the def-use lists of $eflags (to local def-uses) without updating the livein lists of successor blocks:

bb.1.bb1:
  ...
  $eflags = COPY %16:gr32
  JCC_1 %bb.3, 12, implicit $eflags

bb.2.bb1:
; predecessors: %bb.1
  successors: %bb.3(0x80000000); %bb.3(100.00%)
  liveins: $eflags

bb.3.bb1:
; predecessors: %bb.1, %bb.2
  successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
  liveins: $eflags
  %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
  MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
  %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
  JCC_1 %bb.5, 12, implicit $eflags

=>

After X86 EFLAGS copy lowering:

bb.1.bb1:
  TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
  JCC_1 %bb.3, 5, implicit killed $eflags

bb.2.bb1:
; predecessors: %bb.1
  successors: %bb.3(0x80000000); %bb.3(100.00%)
  liveins: $eflags

bb.3.bb1:
; predecessors: %bb.1, %bb.2
  successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
  liveins: $eflags
  %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
  MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
  %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
  TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
  JCC_1 %bb.5, 5, implicit killed $eflags

It seems that since X86FlagsCopyLoweringPass::rewriteCondJmp() does JmpI.findRegisterUseOperand(X86::EFLAGS)->setIsKill(true), it should be safe to assume that EFLAGS is not live out. In that case it should also follow that any successor block will not use a live-in value of EFLAGS. So any successor block should have EFLAGS removed of the live-in list, right?

I am not sure what is the right way to clear EFLAGS of the successor blocks live-in lists. Should it be done in rewriteCondJmp() only or are there other places also that this should be done?

Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.

(Just a reminder that we need to have both performance and code size numbers for this patch. And given that there are a few options, may need a few examples.)

Thanks for sending this out!

A few minor further tweaks.

More fixes from Manuel.

Thanks, updated.

Address feedback from Manuel.

Generally very happy with this. Would like Eric to check this as well to think about the C API and other things that we maybe should be doing while unbuilding this....

FWIW, the adjustments I'm suggesting around tightening the logic can easily be in a follow-up patch if you like. I think generally the code LGTM and I'd just like us to pin down exactly what changes we expect to happen w/ the handles as much as possible to avoid subtle latent bugs creeping in and never getting noticed.

(Tried to get this out last weekend, but was blocked by the Phab down time... Sorry about that ...)

Adding Alina so she is aware of the change and can comment if she spots anything I'm missing...

LGTM to fix folks broken by this.