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.

I was musing some more and it occurred that some values can come from this block

LGTM

LGTM

Could you add dedicated test case?

LGTM

LGTM.

Thanks for fixing this.

@aditya_nandakumar @qcolombet any idea when a better fix for that issue might be available?

@qcolombet Have a look at the associated bug at https://bugs.llvm.org/show_bug.cgi?id=48708, there's a minimized IR reproducer in there.

Could you add a test case exposing the problem?

I agree with @mtrofin's assessment, the previous code was incorrect and if we have a compile time hit, at least short term, I think this is the right thing to do.

Thanks for the follow-up patch!

Nice catch!

For targets that make use of HW modes, register sizes are specific to a subtarget.

Hi Daniel,

I think this is actually a problem GlobalISel does have since it doesn't have the DAG scheduler/linearizer to minimize physical register live ranges. We need an MI equivalent pass to do this

It sounds like we need to have the fast-RA be able to defer to the more sophisticated allocators in certain cases.

To elaborate on the answer:

why is it any more job of the scheduler to keep the live ranges small for the sake of the allocator?

I'm not an expert in this area, but why is it any more job of the scheduler to keep the live ranges small for the sake of the allocator? Shouldn't the allocator be able to deal with any valid input (within computational reason)?

Looks good, thanks fixing the test!

the list scheduler has register pressure tracking disabled I think.

How was this test working before?

Is it something we could fix in the scheduler instead?

LGTM with nit on the function name.

LGTM, just make sure to move getRegisterCostTableIndex under protected.

I'll try to take a look by end of next week.

This is allowed by the current definition. See D85603

I guess the compiling time increase on these benchmarks should be acceptable?