Hey, I don't know if this has anything to do with this change.

Judging from reaction from folks to older diffs, I have a feeling the script may also be running within apple internal builds...

It's not clear from description that it's done to fix those scripts.

Added test for new llvm-link option.

In D128729#3615885, @vitalybuka wrote:

Do we need a test for llvm-link?

For background: We had failing builds because the host clang was configured for a default triple of x86_64-redhat-linux-gnu but the symbolizer build using x86_64-unknown-linux-gnu. I suspect this started failing in combination with the new LLVM_ENABLE_PER_TARGET_RUNTIME_DIR default in LLVM expecting libcxx headers and libraries in a subdirectory matching the target triple.

For background: My understanding of IMPLICIT_DEF is that it was a bit of a workaround/hack necessary because for a given VReg on a control flow join you can have different "undef"/"not undef" merge. Unfortunately being out of SSA form we can't just attach an undef flag to the respective PHI operand as there is no PHI anymore. I always felt a bit uneasy with IMPLICIT_DEF getting used in other situations where they are not at the end of a block just for joining with a different definition/live value in the next block...

I am not sure I understand this change... Wasn't the intention of the existing that code that when you have something like:

%2 = COPY undef %1
  USE %2

that you merge the regs to remove the copy and add an undef to the use operands? =>

USE undef %1

Yes, this could potentially regress rematerializing instructions from target load intrinsics. We would have to pass AA through getTgtMemIntrinsic to allow targets to set the flag themselves.

Maybe we should leave this diff open for a day or two to wait for extra feedback?

I always found it very unfortunate that we keep IR references around in MIR to do alias analysis queries. This appears to remove a lot of those users (all but the ones in the schedule graph construction?), so I highly welcome the change!

Are you sure your bot touble actually comes from GlobalIsel? While GIsel is unfinished and likely broken on x86 it also shouldn‘t be enabled anywhere except a few dedicated globalisel tests…

When there is a bug producing invalid IR then we better crash than hide it.

LGTM

Could you instead exclude the test in the CMakeLists.txt file? Given that this is source from the gcc torture suite that we may want to update again in the future, it's probably best if we can avoid custom changes in the source.

LGTM

LGTM

LGTM

LGTM

I didn't go into any detailed review here, but some top of the head thoughs:

• We already have LiveIntervals which was supposed to be good enough for everyone. I agree that there is no efficient API for querying every vreg live at a certain program point. But I'd feel more comfortable adding a convenience or caching layer for that on top of LiveIntervals...
• We already have a 2nd system LiveVariables (which we failed to actually deprecate enough to remove).
• This is adding a 3rd one, so I think it ought to have some better justification than "simplicity over speed".

Yes that's what I wrote. It seems there is an exception that if for all operands you get readReg() == false (that includes undef) then it is indeed allowed to have a VReg without a definition.

I see this code in the MachineVerifier:

I'm also not sure right now if vregs without definitions are even legal MIR while we are still in MachineSSA... Checking that now

Please choose a better title!

In D125847#3552297, @MaskRay wrote:

Your commit message seems to use the original summary.

@JakeEgan I did a quick look for the code in that stacktrace but nothing jumped out on me. It‘s with inlining effects obscuring it. But I think that crash must be investigated on an AIX machine, there‘s no indication that this directly related to my change which works on all other systems.

LGTM, thanks

oh looks like we also mark the function parameters as live-in causing a bunch of test changes. Anyway I guess this change better reflects the reality of what registers are in the live-in lists. LGTM

The background for this property is that we only really compute the livein lists at the end of register allocation (in the VirtRegRewriter or FastRegAlloc). Before that point using the liveins is pretty much always wrong because they are not complete/correct.

In D125847#3554569, @Jake-Egan wrote:

Hi, this caused arm-float-abi-lto.c to fail on AIX. The failure went away for a few builds, then came back. Could you take a look?

https://lab.llvm.org/buildbot/#/builders/214/builds/1625/steps/6/logs/FAIL__Clang__arm-float-abi-lto_c

In D125847#3551972, @MaskRay wrote:

In D125847#3551841, @MatzeB wrote:

address review feedback. I assume this is accepted and good to push when the buildkit builds look reasonable.

(I changed myself to a blocking review to check the component I mostly care about. I promise I'll perform a quick check.)

address review feedback. I assume this is accepted and good to push when the buildkit builds look reasonable.

There's a lot going on here.

• Could you extract the ShouldAllocClass fixes for fastregalloc into a separate diff so we can get discuss them separately and get feedback from AMDGPU folks who introduced this and are the major other user of this AFAIK.
• I am still wrapping my head around tile registers and tile register configs; specifically I wonder if the support for that really needs to be integrated into the generic register allocation code or whether there is a way to materialize the register configurations in a post-pass. For example do you know how the legacy x86 x87-FPU support works, where we use the register allocator to allocate pseudo FP register fp0-fp7 and then use a post-pass in X86FloatingPoint to insert the necessary stack management operations after the fact. I am not saying it's the same problem, but it is an example of an instance where we managed to have the regalloc allocate to some intermediate pseudo registers and adapt to the complications (in that case register stacks) in a target-specific pass so we wouldn't need to introduce the concept of register stacks to the generic code.

Address review feedback.

llvm/docs/GettingStarted.rst only asks for python >= 3.6 at the moment.

In D124306#3540768, @MatzeB wrote:

This change is breaking the build for me: BooleanOptionalAction is only available in python 3.9!

This change is breaking the build for me: BooleanOptionalAction is only available in python 3.9!

This seems to make sense to me (though I am not super familiar with the code here). Some ideas for clearer code below:

Well I don't have too much experience with STATEPOINTs, but the change makes sense to me so far.

Enable LTO by default and fix a whole bunch of LTO tests because of it.

LGTM, thanks!

LGTM but let's wait for @sameerds for accept.

Changed the approach a bit; summary (and tests) are updated.

While trying to implement things I noticed that -Xclang options are only meant for the LLVM codegeneration part (aka CC1) of clang, they are not meant for the linker/LTO (which would be -Xlinker or -Wl) and consequently there is no easy way in the code to query -Xclang options and pass them on to the linker (nor would it be a good idea I think).

As far as FrameSetup/FrameDestroy are concerned, it's a bug if the targets break on them.

+1 to adding something to lto::Config.

update tests

Therefore, if reserved registers information is changed between last time freezeReservedRegs is called and RAGreedy, it's not picked up by RCI.

Nice catch! LGTM.

In D125847#3521583, @nikic wrote:

Alternatively we could just drop support for LTO with typed pointers :)

I am not completely sure however how to deal with users specifying -Xclang -no-opaque-pointers explicitly. With this change they would end up using opaque-pointers anyway in LTO modes...