This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Divergence driven instruction selection. Part 1.
ClosedPublic

Authored by alex-t on Sep 13 2018, 1:54 AM.

Details

Reviewers
rampitec
dp
Summary

This change is the first part of the AMDGPU target description change. The aim of it is the effective splitting the vector and scalar flows at the selection stage. Selection uses predicate functions based on the framework implemented earlier: https://reviews.llvm.org/D35267

Tests: CodeGen/AMDGPU on Win32

make check-llvm on lnx

Diff Detail

Event Timeline

alex-t created this revision.Sep 13 2018, 1:54 AM
alex-t set the repository for this revision to rL LLVM.Sep 13 2018, 5:54 AM
alex-t added a subscriber: llvm-commits.
rampitec added inline comments.Sep 13 2018, 10:38 AM
lib/Target/AMDGPU/SIInstrInfo.td
1767

You can create enum for the mode in SIInstrInfo.td. Look at SIEncodingFamily as an example.
Then I might be missing something, but I do not see anywhere a value 1 used for mode.

lib/Target/AMDGPU/VOP2Instructions.td
365

Is there a reason to move it from its original place? This seems to grow the patch unnecessary and makes review harder.

402

It would be nice to swap these instructions' blocks back to minimize the diff. Order change is not needed for this patch.

496

Again, yhou probably do not need to move adde and sube patterns.

509

Why is this reg_sequence and extract_subregs are needed? Why not just use i64 operands?

lib/Target/AMDGPU/VOPInstructions.td
576

Where do you set NeedPatGen = 1?

test/CodeGen/AMDGPU/insert_vector_elt.v2i16.ll
450

It is the same, isn't it? Why not to keep it GCN-DAG?

478

Same here.

alex-t updated this revision to Diff 166128.Sep 19 2018, 7:55 AM
alex-t marked 7 inline comments as done.

Source cleanup.

lib/Target/AMDGPU/VOP2Instructions.td
365

In fact the reason was that Tablegen gen-asm-matcher fails with the empty prefix strings

in a def like this the empty string "" overrides the default VOP2_Pseido "prefix" argument that is prefix = "_e32"
VOP2_Pseudo <"v_madmk_f32", VOP_MADMK_F32, [], "">

As a result asm-matcher-gen complained about mnemonic alias with the same string.

For some miracle reason this expressed only with Predicates[] explicitly set.
I don't know how it is related. Some another one Tablegen black magic.

I moved this 4 defs back but removed empty prefix "". Since nothing failed after this I conclude that the empty prefix string was a covered bug.

509

How can I use i64 operands with V_AND_B32 that operates i32?

lib/Target/AMDGPU/VOPInstructions.td
576

Окау. All the stuff related this flag is going to be used later on when I add the patterns for extended encoding forms. I agree that it should be removed from this patch.

rampitec added a reviewer: dp.Sep 19 2018, 2:55 PM
rampitec added inline comments.
lib/Target/AMDGPU/VOP2Instructions.td
365

I think it was supposed to suppress printing of _e32 suffix in a command like:

llvm-mc -arch=amdgcn -mcpu=fiji <<< 'v_madmk_f32 v0, v0, 1.0, v0'

Although we have tests for it in MC/Disassembler/AMDGPU/, so as long as they pass it should be fine.

509

OK, thanks.

test/CodeGen/AMDGPU/llvm.amdgcn.update.dpp.ll
20

This must be a leftover from some experiments, there is no such check.

test/CodeGen/AMDGPU/shift-i128.ll
7

Was it done with update_llc_test_checks.py? It is strange to see indents changed. I suspect the file was really changed manually instead.

alex-t updated this revision to Diff 166260.Sep 20 2018, 4:26 AM

MC/Disassembler/AMDGPU passed
Tests fixed

alex-t marked 2 inline comments as done.Sep 20 2018, 4:26 AM
rampitec accepted this revision.Sep 20 2018, 10:15 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 20 2018, 10:15 AM

Committed r342719.

dp accepted this revision.Sep 21 2018, 6:37 AM
alex-t closed this revision.Sep 26 2018, 9:28 AM
hakzsam added a subscriber: hakzsam.May 9 2019, 2:27 AM

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

arsenm added a comment.May 9 2019, 2:34 AM

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

Did r360293 fix it?

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

Did r360293 fix it?

Yes, it did fix the problem.
Do you plan to backport to LLVM 8?

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

Did r360293 fix it?

arsenm added a comment.May 9 2019, 3:08 AM

Hi there,

This change introduces a regression with RADV and Doom 2016, see https://bugs.freedesktop.org/show_bug.cgi?id=110636
We just discovered this, so LLVM 8 and master are affected. :(
I'm trying to find the root cause, I will let you know.

Did r360293 fix it?

Yes, it did fix the problem.
Do you plan to backport to LLVM 8?

https://bugs.llvm.org/show_bug.cgi?id=41811