Transform illegal intrinsics to V_ILLEGAL

[Leonc]

Summary

Related tasks:

• SWDEV-240194
• SWDEV-309417
• SWDEV-334876

[Leonc]

• Revert changes to return values.
• Refactor code.
• Address comments.

[bcahoon]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h

540 ↗

(On Diff #448074)

Will ILLEGAL be removed?

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

6637

No need for braces

8401

The v_illegal should have an operand for the chain operand in the original instruction, if one exists.

t0 EntryToken
res,ch = intrinsic t0, <other opernads>
->
res = undef
ch = v_illegal t0

Also, if there is a use of the chain definition in the original instruction, then there is no need to add the code below.

[arsenm]

Missing testscases. Also should include some assembler and disassembler tests for V_ILLEGAL

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8401

I'd expect to use the original chain, but it also doesn't really matter given that it's filler anyway

[Leonc]

• Address comments.

[Leonc]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h

540 ↗

(On Diff #448074)

Thanks I missed that one.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

7852

I think this is unnecessary. We still need to call lowerImage for the return value when it succeeds.

8401

Thanks @bcahoon & @arsenm.

I added a check for MemSDNode and pass the chain to V_ILLEGAL if it exists.

[Leonc]

• Handling for atomics plus tests.

[bcahoon]

A couple more tests are needed. A test for for the image intrinsic. Also an assembler test for the v_illegal encoding.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

7827

no braces needed

llvm/lib/Target/AMDGPU/SIInstructions.td

3368

Do we need different version for gfx10?

[Leonc]

• Add different versions of V_ILLEGAL for different targets.

[foad]

In D123693#3581616, @bcahoon wrote:

The rationale is for library code that looks like:

void example(float *p, float v) {
  if (_ISA_verison == 9008 || __ISA_verison == 9010)
     global_atomic_fadd(p, v);
  else
     generic_atomic_fadd(p, v);
}

At optimization levels > 0, the dead code is eliminated, but at optimization level 0, the dead code remain and the compiler attempts to generate an instruction. That instruction would never be executed due to the condition, but it still needs to be generated. In these cases, we don't have multilibs (target specific libraries).

Right, but why can't global_atomic_fadd and generic_atomic_fadd be functions that just contain a call to the corresponding builtin and have an appropriate "target-cpu"= attribute? I guess you would need to do inlining in the backend once you know what subtarget you are compiling for. Is that viable?

llvm/test/CodeGen/AMDGPU/v_illegal-atomics.ll

2

File should not be executable.

[bcahoon]

llvm/lib/Target/AMDGPU/SIInstructions.td

3364

Do we want different mnemonics, or should the same mnemonic be used for the cases when the encoding is all 0's or all 1's?

When you add the llvm-mc tests, it will also be a good idea to add a test that pipes the output of llvm-mc through llvm-objdump to the that the different encodings show up properly for the two cases.

[Leonc]

• Fix indentation, braces, and file mode.

[Leonc]

llvm/lib/Target/AMDGPU/SIInstructions.td

3364

Do we want different mnemonics, or should the same mnemonic be used for the cases when the encoding is all 0's or all 1's?

@arsenm asked for separate definitions:

I would prefer to define a separate V_ILLEGAL that uses all 1s pre gfx10

[Leonc]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8399

Ternary args are the wrong way round. Running local tests before I update the diff to fix this.

[Leonc]

• Fix ternary args.

[Leonc]

• Add v_illegal assembly tests.
• Update instruction definitions.

[Leonc]

llvm/test/MC/AMDGPU/v_illegal-atomics.s

2–3

Adding modifications to pipe the compiler output through objdump as these tests aren't very helpful.

[arsenm]

In D123693#3690864, @foad wrote:

Right, but why can't global_atomic_fadd and generic_atomic_fadd be functions that just contain a call to the corresponding builtin and have an appropriate "target-cpu"= attribute? I guess you would need to do inlining in the backend once you know what subtarget you are compiling for. Is that viable?

The problem is in the called function with the appropriate attributes. The function isn't deleted and we still need to produce something. The current situation is it works in cases where the wrong subtarget also happens to have the same encoding, but breaks on targets that never had an equivalent instruction encoded

[arsenm]

llvm/lib/Target/AMDGPU/SIInstructions.td

3364

The instruction is actually named v_illegal, so you shouldn't invent new names here. I would name these V_ILLEGAL for the gfx10 version and V_ILLEGAL_gfx6_gfx7_gf8_gfx9?

I also think V_ILLEGAL is available in a vop3 encoding (for gfx10) but probably should define that in a separate patch for the benefit of the disassembler

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8401

Checking for MemSDNode doesn't make sense, other nodes can have chains. You would need to pass it in from the source if it applies. However I don't think it's worth the complexity, since this really is filler content anyway

[Leonc]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8401

Thanks, I agree.

[foad]

In D123693#3696457, @arsenm wrote:

In D123693#3690864, @foad wrote:

Right, but why can't global_atomic_fadd and generic_atomic_fadd be functions that just contain a call to the corresponding builtin and have an appropriate "target-cpu"= attribute? I guess you would need to do inlining in the backend once you know what subtarget you are compiling for. Is that viable?

The problem is in the called function with the appropriate attributes. The function isn't deleted and we still need to produce something. The current situation is it works in cases where the wrong subtarget also happens to have the same encoding, but breaks on targets that never had an equivalent instruction encoded

If target-cpu doesn't let you target different CPUs for different functions then I don't understand what it does.

[arsenm]

In D123693#3696493, @foad wrote:

If target-cpu doesn't let you target different CPUs for different functions then I don't understand what it does.

For the relevant cases in the library, we key off the subtarget feature in target-features. The problem is in the final codegen we've propagated the real cpu to the leaf function, which then artificially has an incompatible target-features. We would have to have some code fixup the target-cpu to some random other device with compatible features

[Leonc]

• Address comments.

[Leonc]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8401

It turns out the chain is necessary. An assertion in RemoveNodeFromCSEMaps fails when Op is an atomic intrinsic.

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8401

Yes, a chain is needed but you should be able to use the entry node. If you want to forward it in from the call sites, that would also be OK

[Leonc]

• Address comments and fix tests.

[arsenm]

Could also use a disassembler test

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8397

SDLoc DL(Op)

[Leonc]

• Address comments.

[arsenm]

llvm/test/CodeGen/AMDGPU/v_illegal-atomics.ll

41

This looks backwards. gfx10 is getting the -1 encoding and gfx9 is getting 0

54

gfx10 has v_illegal, so this should be 0x0 and printed as v_illegal

[Leonc]

llvm/test/CodeGen/AMDGPU/v_illegal-atomics.ll

41

Are you sure gfx9 is getting the zero encoding? I thought this line was "address: encoding".
If this was wrong wouldn't the tests in v_illegal-atomics.s also be wrong?

[arsenm]

llvm/test/CodeGen/AMDGPU/v_illegal-atomics.ll

41

You're right, gfx9 looks right. The gfx1030 line with .long 0xffffffff looks wrong.

[Leonc]

llvm/test/CodeGen/AMDGPU/v_illegal-atomics.ll

41

I'm guessing gfx1030 is due to the disassebler. We know the encoding is correct from the tests in v_illegal-atomics.s.
I'll look into it.

[Leonc]

• Address comments.

[arsenm]

LGTM except for the stray debug print. We also probably should clean up the the encoding to rely on subtarget features instead of the generation check

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8396

DRop this debug printing

[Leonc]

• Remove debug code.

[Leonc]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

8396

Thanks, my mistake.

[Leonc]

In D123693#3703089, @arsenm wrote:

We also probably should clean up the the encoding to rely on subtarget features instead of the generation check

Is there a generic way to do that without adding maintenance overhead, or is it better that we don't implicitly support new subtargets/generations in case they have different encodings of v_illegal?