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