Thank you! :)

@aadsm Is there any update on the regression fix? This patch also breaks the C++ module test suite, so i would prefer if we could revert it if the fix takes longer.

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

In D56151#1557861, @jmorse wrote:

Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?

The patch effectively makes the codegen backend stricter about what DBG_VALUE insts it will accept, i.e. they have to refer to vregs that are alive. If it helps, this is already something that's enforced a few optimisation passes later in LiveDebugVariables [0], this patch just brings that check further forwards, due to the errors that can be introduced (described above). What's unfortunate is that this also prevents occasions where register coalescing re-enlivens DBG_VALUEs with a vreg that has the right value in it, occasions which can be generated due to limitations in SelectionDAG.

This then becomes a trade-off (see immediately preceeding comment) of whether the problem is so bad that we should trade some lost locations for getting rid of placeDbgValues. IMHO: yes, because we're trading definitely-incorrect locations for some more being optimized-out, a net reduction in error, to me. Other opinions most welcome.

[0] https://github.com/llvm/llvm-project/blob/b9f1e7b16ed2341e54b4e2033d111e7a2ca19b9a/llvm/lib/CodeGen/LiveDebugVariables.cpp#L624

The third instruction in the function looks like it is using r6 as the base pointer to save one of the argument registers on the stack, but r6 isn't set until further down.

Revert multi-range support. Add early hit detection (before children) instead.
Add more tests.

• clang-format all the code. If it's ugly, don't complain to me

@craig.topper, hmm, what happens in terms of CG when LAHF/SAHF are not available? I assume its just worse CG as you could spill AH onto the stack and do a load/store. This actually results in library calls which may not be possible to fulfill.

LGTM

lgtm, the serialized types are uint32_t:

struct alignas(32) XRayRecord {
...
  // The thread ID for the currently running thread.
  uint32_t TId = 0;

In D63551#1557215, @rovka wrote:

Hi Amara,

Is something like this OK for intrinsics: https://reviews.llvm.org/differential/diff/206394/ ? It has the disadvantage that if we have an intrinsic with 2 aggregate args, it will be hard to tell where one of them ends and the next one begins. OTOH, I'm having trouble finding intrinsics with even one aggregate parameter, and in fact just using getOrCreateVReg there doesn't cause check-all to crash (nor the test-suite, nor selfhost at -O0 on AArch64).

Alternatively, we could preserve the old interface here and try to move pack/unpackRegs to utils, where we can use them from both the IRTranslator and CallLowering.

Either way, it would be nice to be able to add a test-case before changing this interface, but like I said I don't really know my way around intrinsics that well. Any suggestions?

LGTM

In D61340#1553136, @MikeDvorskiy wrote:

Louis,
thanks for the alternative proposal.

A couple of question:

1. What's motivation to change "stdlib" to "extra"? To avoid some confusing? (because "stdlib" is not a part of the standard library here)
2. Can we keep "pstl" name (instead of proposed "include")? The fact is many people have already use PSTL code (as "third party" headers) and a usage model is following:
   
   #include "pstl/execution" #include "pstl/algorithm" #include "pstl/numeric" #include "pstl/memory"
   
   We would not like to force them changing #include "pstl/execution" to #include "include/execution"

LGTM. Thank you!