Details
- Reviewers
kzhuravl msearles jhenderson - Commits
- rG9ee272f13d88: [AMDGPU] Add gfx1030 target
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| 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 | |
| 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? | |
| 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. | |
LGTM. This was reviewed downstream. Additional review feedback should be addressed in the follow up patch to simplify the merge downstream. Thank you.
| llvm/docs/AMDGPUUsage.rst | ||
|---|---|---|
| 266–267 | Seems odd to list xnack as a "supported fetaure" when the hardware doesn't support it. | |
| llvm/docs/AMDGPUUsage.rst | ||
|---|---|---|
| 266–267 | Yeah, it is off, but we may want to review the whole table. | |
| 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? | |
| 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. | |
Unrelated, but I think we should stop emitting the features in the target-features string if it's present on the base device