♟ rampitec

You probably need to extend the pass to support subregs, not necessarily in the same patch.
But you definitely need a mir test with subregs used with either operands.

[AMDGPU] Assert in getOpSize() there are no sub-dword subregs

Addressed review comments.

[AMDGPU] Fixed SIInstrInfo::getOpSize to handle subregs

Split off independent D52736.

[AMDGPU] Fold copy (copy vgpr)

[AMDGPU] Fix ds combine with subregs

[AMDGPU] Remove useless check from test. NFC.

[AMDGPU] Initialize instruction itinerary from GCNSubtarget

Thanks! Looks good, but please wait for others to review.

In D51794#1233163, @arsenm wrote:

Considering emitting traps requires fixing traps first, otherwise a program that should work will incorrectly trap

LGTM except for small formatting issue.

LGTM.
A note though, I would think of it as a some subset of flat.

In D51794#1228453, @arsenm wrote:

In D51794#1228246, @rampitec wrote:

In D51794#1228202, @arsenm wrote:

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

A standard reaction to pure virtual call is a runtime error message and abort. We cannot produce an error message from kernel but we can abort. I would suggest to lower it to s_trap.

I don't think this is how that's implemented. Other targets generate a literal call to null in this case. This is an edge case since typically undef/null calls are turned into unreachable in simplifycfg, which there is a flag to generate a trap on separately.

In D51794#1228202, @arsenm wrote:

HCC had a bug recently that was producing calls to under. I assume a call to null would appear for a pure virtual call. They are undefined to execute, but they could appear in dead code that is never reached so it’s not an error to have them exist

In D51794#1228186, @arsenm wrote:

There is no error to hide. This is perfectly valid IR

What's the point of hiding an error? We should rather produce a normal error message.

Looks good, but Matt needs to approve.
Also do you need a test?

Please attach full context diff.

[AMDGPU] Split v32i32 loads

I am not sure that a copy + e32 instruction is better than a single e64 instruction. In fact I think it is worse.

It is not really an NFC. It changes codegen even if you cannot forge such an IR right now. You cannot write a test for an NFC. NFC is something like changing comment or formatting, or changing a vector with list. Also NFC does not usually need a review. Please change subject.

rampitec (Stanislav Mekhanoshin)
User

Projects

User Details

Recent Activity
View All

Fri, Oct 19

Thu, Oct 18

Tue, Oct 2

Mon, Oct 1

Fri, Sep 28

Thu, Sep 27

Wed, Sep 26

Tue, Sep 25

Mon, Sep 24

Fri, Sep 21

Sep 20 2018

Sep 19 2018

Sep 17 2018

Sep 14 2018

Sep 13 2018

Sep 12 2018

Sep 11 2018

Sep 10 2018

Sep 8 2018

Sep 7 2018

Sep 6 2018

Sep 4 2018

Aug 31 2018

Aug 30 2018

Aug 29 2018

Aug 28 2018

rampitec (Stanislav Mekhanoshin)User