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?

Thanks all for your help and @nikic in particular for doing the perf measurements and pushing for a better solution.
I think this shows that reviews work and produce better code quality!

• Use Known directly instead of using an intermediate AddrKnownBits variable
• Change AccCstIndices into AccConstIndices
• Move initialization code of IndexBits next to first use
• Regenerate test case using update_test_check

Oups, tagged Eli's old account.
Tagging the new one @efriedma

Did 1 & 2 of @eli.friedman's suggestions.

• Skip zero indices
• Accumulate constant indices with constant scaling factor in a separate variable to avoid calls to computeForAddSub

Thanks @efriedma for the suggestions!

If we remove that from the equation, I expect the compile-time impact of this patch would be pretty minimal. I started some work on this, but I'm not sure when I'll be able to finish it.

Looks reasonable but I'd like to have an idea of the compile time impact of this patch.

Following the conversation in D87342 , this patch has been rebased and all the previous feedbacks have been addressed.

Get rid of TrailZ and track everything with AddrKnownBits.

Rebase patch
Add the reviewers from D87342

The general direction is fine, but IMHO there are needless code churn.
Why can't we stick to getCostPerUse instead of changing all the users?

Nice catch!

Thanks for the quick review @aemerson!

Could use a test, but it generally makes sense to have such a method.

• Add a test for the new method

Thanks guys for the quick review!

• Move method earlier in the header
• Fix comment in test

• Add getMinValue/getMaxValue

Abandoning this diff in favor of D86364 based on our discussion.