I looked and don't see anything for this. We fold all of these in separate MI passes

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!

I'm not sure what context this gets called in. Maybe it's possible to hit this for DPP instructions?

Do you know if AMDGPU has any convergent instruction involved in pattern with folding?
I could use that to create a test case.

Nice catch!

But that's not a problem of this patch, and we will address that later if needed.

@fhahn are you okay with the clang tests update as well?

Hi Jay,

Thanks for the heads-up and the revert.

• includes fixes to clang tests that were missed in the original commit.

Alright, at first I thought it was a latent bug in the dead argument elimination pass and now I think the two failing tests just need updating.
I'll upload a diff with the fix tests. (See below for the diff of the tests themselves.)

Ah thanks for the heads-up, looking at the clang issue.

I see what you are trying to solve, but at the same time, in theory any instruction could have this kind of register pressure problems. (Though, we probably don't ever create too many not spillable live-ranges around regular instructions and regular instructions have a reasonable number of operands.)

Nice catch!

The verifier doesn't like that at all since it doesn't have an associated class. Plus every target would have to handle encoding it as something in the end

The patch itself looks good but I wonder if we could maybe take a simpler road:
Could we assign register zero (noreg) to this failed vreg?

At first glance it looks similar to what @aditya_nandakumar implemented internally to get rid of copies produced by eviction chains.

Thanks @arsenm for tracking this down and the fix!

Hmm, that's weird, if one register fails within the same level, we should roll back the whole level to its previous state.

I now have another attempt which tracks a stack of successful recolorings that are rolled back on failure

I'm wondering if a better strategy would be to try avoiding recolor candidates that could conflict with the saved recoloring state for fixed registers

Hi Jonas,

The complicated cases are indeed not well tested since it is very hard to hit them. I was hoping Matt's fix would avoid falling in one of them, while not preventing actual success. Looks like it wasn't the case.

I haven't looked into it yet, but we see cases of "ran out of registers during register allocation" with this patch, for code that compiled succesfully before.

The fix looks good to me.

• Set the default smallvector size to 10

LGTM, nits below

Hi Mircea,

should we go ahead and revert (most of*) https://reviews.llvm.org/D35816?

I'm also guessing that you'd want to avoid using string keys for the function-attribute implementation?

Let me just step back a little bit and say that now that I think about what we did, having something that answers "should I run in this instance" is desirable, the implementation doesn't really matter. We did it with function attributes, but having a bisect client API like you're introducing is fine.
My only complain is that the client interface should not have remote in the name :P.

Hi Amara,

Hi Amara,

I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.

This seems accidental. I think ideally we would enforce consistency of the flag within an instruction. Short of that we better not assume anything like just the first operand having a kill flag...

Makes sense, thanks!

This mostly seems to benefit X86

One last comment, otherwise LGTM.

LGTM

Looks good to me as well.

Given that, can you approve Philip's patch?

Following the conversation in https://reviews.llvm.org/D105723#2894209, approving this patch.

Hi all,

LGTM

I've tried approaches with target pseudo intrinsics and extra IR passes, but still everything depends on missing the external (relatively to the IRTranslator) way to obtain the info about Value->Vreg mapping.

I am not sure I understand the issue here.
Could you paste the IR with the wrong transformation (before this patch) for one of the test (e.g., the smaller one)?

I'm suspicious of how simple this patch is, and not sure why this special phi handling is needed given that refineSubRanges was already called

Looks fine to me but please wait on the comment from Jay (@foad ) and Matt (@arsenm ) before pushing.
Nitpick below.

Hi Amara,

Hi Nick,

By removing the reportTranslationError() temporarily in order to test my implementation, that would cause errors in other targets. Can you please suggest me any other way to solve this issue?

LGTM

LLVM ERROR: unable to translate in big endian mode (in function: f)

Add GlobalISel infrastructure up to the point where we can select a ret void.

I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:

%1 = bitcast i8* %0 to %struct.ST*
In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.

LGTM.
Disclaimer: I didn't really look at the AMDGPU changes.