-
Notifications
You must be signed in to change notification settings - Fork 27
[lit][offloader][ninja] Add ability to toggle gpu by name #646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This change allows you to set an enviornment variable to pick which gpu the offloader will use. For example say I have an Intel GPU I can do: ``` set "OFFLOADTEST_GPU_NAME=Intel(R) Arc(TM) A770 Graphics" && ninja -C ..\llvm-build\ check-hlsl-clang-d3d12 ``` Or if I just want to specify the vendor I can do ``` set "OFFLOADTEST_GPU_NAME=Intel" && ninja -C ..\llvm-build\ check-hlsl-clang-d3d12 ``` I am also adding a way to use the adapter index because in testing I found typign out even just the substring can be more annoying than a single char for the gpu index. Note GPU index is not stable. In my testing of EGPUs if your gpu unplugs the numbering scheme just increments until you restart your machine.
| if ShouldSearchByGPuName and is_gpu_name_match: | ||
| target_device = device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason we have to pick the right target device in lit and not just set the offloader args is because setDeviceFeatures checks vendor names to turn on\off vendor specifc tests and we need this to match what we are telling the offloader.
| bool EnableDebugLayer = false; | ||
| bool EnableValidationLayer = false; | ||
| int AdapterIndex = -1; | ||
| std::string AdapterSubstring = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the right name is for this. It can techically be the whole Device Description name. It can also be a subset like the Vendor name i.e (Intel, Nvidia, AMD, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In DXGI_ADAPTER_DESC1 it is called Description, so maybe AdapterDescription would be a good fit, or simply just Adapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a name to indicate it could be a substring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to store the device description in the DeviceConfig? The device itself stores the description, it's set in DXDevice::create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string isn’t necessarily the device description. It can be a substring. It can also be lowercased or uppercased or any casing. Ie not a direct match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The device itself stores the string from querying the device driver. This is just the string we get from the offloader cli flags so we know what to search for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we didn't put this information into the DeviceConfig struct, since these are really the filtering predicate.
Why don't we instead create some sort of predicate object and have an optional default empty predicate?
| struct DeviceConfig { | ||
| bool EnableDebugLayer = false; | ||
| bool EnableValidationLayer = false; | ||
| int AdapterIndex = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the index we use for GetAdapter hence the simplest name was just AdapterIndex.
hekota
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| bool EnableDebugLayer = false; | ||
| bool EnableValidationLayer = false; | ||
| int AdapterIndex = -1; | ||
| std::string AdapterSubstring = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In DXGI_ADAPTER_DESC1 it is called Description, so maybe AdapterDescription would be a good fit, or simply just Adapter.
llvm-beanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you decide to do the filtering on the creation of the device instead of the selection. Filtering on selected device is API-agnostic, but this implementation needs to be done per API backend.
| bool EnableDebugLayer = false; | ||
| bool EnableValidationLayer = false; | ||
| int AdapterIndex = -1; | ||
| std::string AdapterSubstring = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to store the device description in the DeviceConfig? The device itself stores the description, it's set in DXDevice::create.
|
|
||
| # Environment equivalents (useful for ninja): | ||
| # OFFLOADTEST_GPU_NAME | ||
| GPUName = os.environ.get("OFFLOADTEST_GPU_NAME", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this could also be a CMake-provided value. That could also make it possible to have CMake generate configurations for more than one device from a single configuration tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that. One reason i did it this way was cmake changes causes me to have to do full recompiles so i try and avoid them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm actually thinking about this a bit more I want the toggling between gpus to be cheap and passing in strings via cmake makes this expensive since you are changing the build system which forces a recompile. Maybe it would be better as a lit argument?
Thats a good point. At the time I wrote it I was thinking it was wasteful to create devices we weren't going to use so just create the one we want to use. |
| bool EnableDebugLayer = false; | ||
| bool EnableValidationLayer = false; | ||
| int AdapterIndex = -1; | ||
| std::string AdapterSubstring = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we didn't put this information into the DeviceConfig struct, since these are really the filtering predicate.
Why don't we instead create some sort of predicate object and have an optional default empty predicate?
| return std::string(DescVec.data()); | ||
| } | ||
|
|
||
| static bool icontains(std::string Hay, std::string Needle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than string compares, why not allow the predicate to be a regex and use llvm::Regex?
| // If the user requested a specific adapter by substring, pick the first | ||
| // match. | ||
| if (!Config.AdapterSubstring.empty()) { | ||
| for (uint32_t I = 0; I < Count; ++I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to collapse this loop and the loop below into one loop. Instead of the current pattern which is something like:
if (filtering by name)
loop devices
compare predicate
add device
return
else if( filtering by index)
add device by index
return
else
loop devices
add device
What if instead we just did:
loop devices
if (!predicate applies)
continue;
add device
Then we can have an abstraction to apply the predicate (either comparing the index or regex match).
This change allows you to set an enviornment variable to pick which gpu the offloader will use. For example say I have an Intel GPU I can do:
Or if I just want to specify the vendor I can do
I am also adding a way to use the adapter index because in testing I found typign out even just the substring can be more annoying than a single char for the gpu index. Note GPU index is not stable. In my testing of EGPUs if your gpu unplugs the numbering scheme just increments until you restart your machine.
Not sure if this change just useful for me to pick between multiple discrete gpus on a single rig, egpus, and integrated gpus but I suspect the discrete vs integrated picking will be useful for most.