[SYCL] Add more aspect information for intel_gpu_* in device config file #14188
Conversation
| //def : TargetInfo<"Test2", [AspectCpu] # AllUSMAspects, []>; | ||
|
|
||
| // TODO: The aspects listed for the intel_gpu targets right now are incomplete; | ||
| // only the fp16/fp64/atomic64 aspects are listed. |
There was a problem hiding this comment.
May be update comment to include SG aspects? Also, is the list of GPUs complete? Thanks
There was a problem hiding this comment.
I agree about SG sizes comment update.
Also, is the list of GPUs complete?
Likely no, we have almost 40 lines describing different Intel GPU architectures in #13976, not counting aliases
I think that we need some kind of an integration test here between clang driver, SYCL headers and device config file. All those places list known targets and we want them to be in sync. However, I wouldn't block this commit by lack of such test, but that is something we need to have going forward. The fact that this file is expanded incrementally is expected, I think
asudarsa
left a comment
asudarsa
left a comment
There was a problem hiding this comment.
LGTM. Is this supported by any existing tests? Will we benefit for adding tests here? I am ok with adding test in a subsequent PR.
Thanks
|
Hi, Any comments about the failing tests? Thanks |
There is a couple of tests: We won't be able to test every config line, simply because we don't have all that HW in our CI pool. However, we should add a test (I propose as a follow-up PR) to ensure consistency between targets listed in the config file, targets available through |
| defvar IntelBaseAspects = [AspectExt_intel_esimd]; | ||
| class IntelTarget<string Name, list<Aspect> Aspects, list<int> subGroupSizesList> | ||
| : TargetInfo<Name, IntelBaseAspects # Aspects, subGroupSizesList>; | ||
| def : IntelTarget<"intel_gpu_pvc", Fp16Fp64Atomic64, Sg16_32>; |
There was a problem hiding this comment.
Please also update comment here , instead when one will add a new architecture, llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td will not be updated
| defvar IntelBaseAspects = [AspectExt_intel_esimd]; | ||
| class IntelTarget<string Name, list<Aspect> Aspects, list<int> subGroupSizesList> | ||
| : TargetInfo<Name, IntelBaseAspects # Aspects, subGroupSizesList>; | ||
| def : IntelTarget<"intel_gpu_pvc", Fp16Fp64Atomic64, Sg16_32>; |
There was a problem hiding this comment.
Why there are no _pvc_wg and other newer architectures? Should we add them?
There was a problem hiding this comment.
I've added some of this missing targets - see here. For these targets, ocloc has them under different names for some reason (e.g. ocloc would not recognize -device pvc_vg), so I had to add some extra handling in the driver. For intel_gpu_lnl_m and intel_gpu_bmg_g21, ocloc does not seem to accept lnl_m or bmg_g21 (or even 20.4.41 or 20.4.1, the numeric values of those architectures) as values for -device yet. It could be that my environment also has an older ocloc version, but either way, I think I will address those architectures in a later PR.
Co-authored-by: Dmitry Vodopyanov <[email protected]>
Follow up to #14188 adding LNL and BMG target info.
Follow up to intel#14188 adding LNL and BMG target info.
7 participants
No description provided.