I think this can catch bugs earlier, LGTM.

In D147158#4231787, @rampitec wrote:

In D147158#4231755, @Joe_Nash wrote:

In D147158#4231480, @rampitec wrote:

I am not so sure about SGPRs. I believe it is legal to use SGPR halves.

It appears that this patch does not change anything about the legality of allocating registers in class SGPR_LO16.

But if we will allow SGPRs this code will be needed on older targets again.

SGPR_HI16 is already marked as not allocatable. So L611 was partially redundant with that. But this patch does appear to remove a barrier to allocating into the hi or lo half of TMA_LO or some other non-GPR SReg.
I guess there is other code somewhere preventing allocating to those non-GPR SReg? If so, this patch can be marked NFC and the commit reworded to say it skips redundant reservations.

LGTM

This looks like it fixes 49830, and I agree with foad it is useful in the AMDGPU backend. Functionally LGTM! Thanks for working on it.

I'm fine with the patch, but it's a shame the solution isn't more robust.
Its not self-evident why runline is written like that, so I expect uses without the ':' could creep back in.

In D144271#4140757, @foad wrote:

In D144271#4139988, @Joe_Nash wrote:

In D144271#4139981, @arsenm wrote:

In D144271#4139979, @Joe_Nash wrote:

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

I guess the spec was changed between gfx9 and gfx10. On gfx10 and newer the docs say modifiers are supported on VOP1 as well as VOP3. But in terms of what our backend would make with just the above patch, it seems the modifiers would be on v_mov_b32_e64 but not v_mov_b32_e32. Hence my ask to check the tablegen records.

There are no modifiers in VOP1 - there are no bits in the encoding for them.

1. There is a bug in permlane16 selection I am working on and will put up a patch for soon
2. I'm curious how AMDGPU/permlane-op-sel.ll test passes. When I run the runline "llc -march=amdgcn -mcpu=gfx1030 -show-mc-encoding" using ToT and check the output I get "v_permlane16_b32 v0, v0, s7, s0 op_sel:[1,0] ; encoding: [0x00,0x49,0x77,0xd7,0x00,0x0f,0x00,0x20]" not what is in the check line.

In D144271#4139981, @arsenm wrote:

In D144271#4139979, @Joe_Nash wrote:

Actually, I think due to the way the operand lists for _e32 instructions are constructed, the modifiers are not taking effect.
If you dump the tablegen records, if the change was effective there should be a src0_modifiers operand on the V_MOV_B32_e32 instruction.

I'd expect it to only be on v_mov_b32_e64?

Thanks for taking a look at this issue.

LGTM besides my previously noted typo, but please wait for @arsenm

LGTM

Thanks, LGTM

Overall I like the patch. It definitely improves readability.

removed getAsm64

Further cleanups are possible, such as deleting one of these fields completely or cleaning up AsmVOP3OpSel, but I'd prefer to do it incrementally.

In D138870#4020211, @arsenm wrote:

In D138870#4020204, @Joe_Nash wrote:

The code looks fine, but as you say, the change visible in user code and could break something. Do you want to handle that somehow? Maybe wait for @b-sumner

OpenMP assumes flat pointers all over, so if someone was relying on this behavior it wasn't doing anything useful. They had dead code

In D139000#4025199, @Pierre-vh wrote:

Last time I checked (before the holidays), it builds fine without the patch reverted. I can try reverting the patch later when this lands perhaps? Or should it be done at the same time?
For reference the only build issues I experienced where with hard f64 functions like llvm.sin.f64, which are apparently not (well) supported if I understand correctly

The code looks fine, but as you say, the change visible in user code and could break something. Do you want to handle that somehow? Maybe wait for @b-sumner

LGTM! Thanks for catching that.

LGTM

LGTM

The last functional status I see here is.

LGTM

precommit gfx11 runline and rebase

Overall this is looking quite good.

do this on an "opt-in" basis

Makes sense to me

LGTM

In D139000#3962770, @Pierre-vh wrote:

In D139000#3960912, @Joe_Nash wrote:

Overall this looks pretty good. As you say, the feature checking logic is quite limited, but that's not a problem.
I think after this patch lands https://reviews.llvm.org/D123693 can be reverted. Can you try to revert that with this patch and check if device libs can be built correctly at -O0?

I did a quick test where I passed all .bc files from the device libs to Clang for fiji (gfx8) and it still doesn't build even with this patch. The pass kicks in a few times but there's some issues with "dot" instructions.
Not sure how to address those - should it be done in this pass? For instance device libs has a few places here it uses (target("dot8-insts") which allows selection to work (because that only checks the feature) but then it fails because there is no "real" instruction for GFX8 dot8, only GFX11 (it uses the generation).

Do I just go "whack a mole" and try to build, add more checks, try to build again, etc?
I'm worried about complexity exploding if the checks need to be more intricate. e.g. I see that dot instructions have been introduced in the middle of the GFX9 generation (GFX908?) so I'd already need to change the pass completely to check for GFX908

Ok, apart from that comment LGTM.

Overall this looks pretty good. As you say, the feature checking logic is quite limited, but that's not a problem.
I think after this patch lands https://reviews.llvm.org/D123693 can be reverted. Can you try to revert that with this patch and check if device libs can be built correctly at -O0?

Thanks, LGTM

LGTM

Its not just SGPRs we want to disable, right? Its inline and literal operands too. So the commit title is slightly misleading.

Thanks to Jay as well for pointing out std::optional on another review.
LGTM

I believe the latest guidance is to use std::Optional instead of llvm::Optional (https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716). Also it makes a difference in tests, so is not NFC. Otherwise looks good.

LGTM

In D133012#3934097, @foad wrote:

I have often wanted a standard way of mapping two different pseudos to the same real instruction.

Thanks, LGTM.

In D133012#3933756, @mbrkusanin wrote:

• By overriding opName (or PseudoInstr of tablegen record) for _strict pseudo we can have both pseudos select into same real instruction, so now we don't need two.

This is a creative use of the opName, but I think it probably hinders readability. No other instructions are doing this, so when I look at definition of the Real instruction it seems to refer to the non-strict pseudo.

LGTM

See my comment, otherwise I like it.

In D137952#3927187, @dp wrote:

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

In D137952#3925471, @dp wrote:

Why does it change the error position?

Before this change getParsedSrcIndex handled tied operands as if they were parsed and returned dst index for tied operands:

unsigned getParsedSrcIndex(unsigned SrcIdx, bool ComponentHasSrc2Acc) const {
  if (ComponentHasSrc2Acc && SrcIdx == (MAX_SRC_NUM - 1))
    return getParsedDstIndex();
  ...
}

This patch corrects handling of parsed operands: now they don't include tied ones.

This looks like a reasonable refactor. Why does it change the error position?

LGTM

LGTM

LGTM

LGTM

This part of the commit message is worded confusingly for me.

LGTM

In D136148#3883668, @dp wrote:

The patch may be useful, but I’m concerned with file naming. I think the naming should be consistent. Right now we have vop3, vop3c, vop3cx and now vop3_from…. Maybe we should think over test naming?

IMO vop3_from_vop2_dpp16 is questionable.

To tell you the truth, I considered splitting VOP3 opcodes into native and promoted parts but was unable to find good file names and abandoned the idea.

Any proposals to make the naming consistent?

use const auto &, hoist loop invariant code, shorten td line length, delete parameter name underscores

LGTM

Overall looks good. Did you observe any instructions that assembled when they shouldn't? It looks like only differences in error message from the tests.

LGTM