Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Needs tests
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 449 | There's no point to having this since you never set it to anything else | |
| 454 | Ideally this would handle vectors like <2 x i8> | |
| 455 | The address space check should be first | |
| 459 | The builder already has a getInt32Ty | |
| 461 | I.getPointerOperand | |
| 469 | This should also skip volatile loads | |
- Address code reviews. Looks like adding "isDereferenceableAndAlignedPointer" is too strong to prevent expected code transformations.
- Modify related LIT tests.
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 452 | This one (VT && VT->getBitWidth() < 32) is able to handle vectors with bitwidth < 32 or scalar (!VT). Are you saying to use DataLayout to handle the pointer dereferenceable issue? | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 449 | No space after * | |
| 451 | M->getDataLayout() | |
| 462–475 | You don't need an entire block of code that is mostly the same for the vector case. The non-vector case requires a bitcast as well. | |
| test/CodeGen/AMDGPU/widen_extending_scalar_loads.ll | ||
| 3 | These should be running just this pass with opt | |
| 12 | This should not be converted because you don't know if it's 4 byte aligned | |
| 48 | Needs tests with half and <2 x half>, i1, and maybe another exotic size. I'm pretty sure this will assert for half as is now. Also need tests with various alignments and volatile | |
| test/CodeGen/AMDGPU/widen_extending_scalar_loads.ll | ||
|---|---|---|
| 48 | Also would be good to have a test specifically loading from the dispatch packet like happens in the workitem ID calculation | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 42 | Included twice. | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 456 | Move align check before DA. It would also be better to move the datalayout alignment check into a helper function checked here | |
| test/CodeGen/AMDGPU/widen_extending_scalar_loads.ll | ||
| 6 | Don't use FUNC, you don't have this as a check prefix | |
| 25–28 | This isn't checking the relevant parts | |
| 134 | _f16 is the naming convention | |
| 146 | _v2f16 | |
| 162 | Needs to check the type | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 131–136 | The comment explains too specifically what it is doing, it should be describing intent and why. | |
| test/CodeGen/AMDGPU/widen_extending_scalar_loads.ll | ||
| 2 | These are IR check lines, so shouldn't use GCN. You also don't need or use the HSA check prefix | |
| 89–91 | This should check the integer type/operands | |
| 173 | Spelling _addrespace1 | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 230 | This doesn't actually do the widening, so the name should be something like canWidenScalarExtLoad | |
| lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp | ||
|---|---|---|
| 237 | This should really be I.isSimple(), in case it's atomic. | |
Included twice.