This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add gfx1030 target
ClosedPublic

Authored by rampitec on Jun 15 2020, 2:42 PM.

Diff Detail

Event Timeline

rampitec created this revision.Jun 15 2020, 2:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
arsenm added inline comments.Jun 15 2020, 3:03 PM
clang/lib/Basic/Targets/AMDGPU.cpp
174

Unrelated, but I think we should stop emitting the features in the target-features string if it's present on the base device

llvm/lib/Target/AMDGPU/AMDGPU.td
396

We don't use underscores in any other subtarget feature name. change to -?

500

Does this need its own feature apart from the instruction set?

512

mad-f32-insts may be a sufficient name?

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
926–930

I prefer to use positive conditions and swap the select operand order

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1139

Volatile should not be necessary (at least not unconditionally)

llvm/lib/Target/AMDGPU/SIShrinkInstructions.cpp
223

It seems like a bug this would be missing?

llvm/lib/Target/AMDGPU/SOPInstructions.td
799–800

This is a problem. ReadCycleCounter should probably be marked IntrNoMem. I've also been thinking we need to introduce memory free versions of setreg pseudos that only touch the mode register

llvm/test/CodeGen/AMDGPU/GlobalISel/urem.i64.ll
3119

This shouldn't have changed

arsenm added inline comments.Jun 15 2020, 3:12 PM
llvm/lib/Target/AMDGPU/SOPInstructions.td
799–800

s_getreg should definitely not have side effects. it's already a problem that it is mayLoad. We don't get CSE of addrspacecast apertures because of this. Is marking readcyclecounter IntrInaccessibleMemOnly enough to fix this?

rampitec marked 7 inline comments as done.Jun 15 2020, 3:14 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/AMDGPU.td
396

OK, in a follow-up patch.

500

Yes, it has been dropped.

512

OK, in a follow-up patch.

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
926–930

OK, in a follow-up patch.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1139

We add it for other atomics here.

llvm/lib/Target/AMDGPU/SOPInstructions.td
799–800

Well, since it now reads timer... We can try to figure it out in subsequent patches.

llvm/test/CodeGen/AMDGPU/GlobalISel/urem.i64.ll
3119

This is changed because there is no -mcpu in the test.

kzhuravl accepted this revision.Jun 15 2020, 4:13 PM

LGTM. This was reviewed downstream. Additional review feedback should be addressed in the follow up patch to simplify the merge downstream. Thank you.

This revision is now accepted and ready to land.Jun 15 2020, 4:13 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 4:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
foad added a subscriber: foad.Jun 17 2020, 1:07 AM
foad added inline comments.
llvm/docs/AMDGPUUsage.rst
266–267

Seems odd to list xnack as a "supported fetaure" when the hardware doesn't support it.

rampitec marked an inline comment as done.Jun 17 2020, 9:12 AM
rampitec added inline comments.
llvm/docs/AMDGPUUsage.rst
266–267

Yeah, it is off, but we may want to review the whole table.

foad added inline comments.Jun 25 2021, 9:02 AM
llvm/lib/Target/AMDGPU/AMDGPU.td
1245

The ! is obviously wrong in this definition, but if I remove it, all the tests still pass. So does this predicate actually control anything?

rampitec added inline comments.Jun 25 2021, 10:35 AM
llvm/lib/Target/AMDGPU/AMDGPU.td
1245

We don't select these, so only AssemblerPredicate is actually used. This is an obvious typo to fix.