[UR][L0] Propagate OOM errors from USMAllocationMakeResident

[0x12CC]

This change ensures that USM allocation APIs don't return UR_RESULT_SUCCESS when an out of memory (OOM) error occurs within USMAllocationMakeResident. It's similar to the change reverted in #972, but only forwards OOM error results to avoid causing regressions; The behavior for other result values is unchanged.

[0x12CC]

@kbenzie, @jandres742, this PR should address the regressions discussed in intel/llvm#11312. I was able to reproduce the failures on an Intel Arc A770 device and they're related to other result values we don't handle.

Tests for this PR are in intel/llvm#11696.

[jandres742]

@kbenzie, @jandres742, this PR should address the regressions discussed in intel/llvm#11312. I was able to reproduce the failures on an Intel Arc A770 device and they're related to other result values we don't handle.

Tests for this PR are in intel/llvm#11696.

hi @0x12CC . Your changes have now:

  auto Result = USMAllocationMakeResident(USMSharedAllocationForceResidency,
                                          Context, Device, *ResultPtr, Size);
  if (Result == UR_RESULT_ERROR_OUT_OF_DEVICE_MEMORY ||
      Result == UR_RESULT_ERROR_OUT_OF_DEVICE_MEMORY) {
    return Result;
  }

so it seems that if an error other than OUT_OF_DEVICE_MEMORY is returned, we need to mask it with SUCCESS, because we dont handle those errors. Could you elaborate on what cases you are seeing the error? I would say it is better we handle correctly those cases rather than masking the error with SUCCESS.

[0x12CC]

so it seems that if an error other than OUT_OF_DEVICE_MEMORY is returned, we need to mask it with SUCCESS, because we dont handle those errors. Could you elaborate on what cases you are seeing the error?

The matrix tests failing on Intel Arc are due to an unhandled error result. The call to zeContextMakeMemoryResident returns ZE_RESULT_ERROR_INVALID_ARGUMENT. This seems like a bug in L0 since zeContextMakeMemoryResident doesn't specify this value as a possible result. The argument values are not null so I'm not sure if there's anything we can do to handle this error.

I would say it is better we handle correctly those cases rather than masking the error with SUCCESS.

I agree. The current implementation masks all errors with success. The change I'm suggesting in this PR is to forward OOM errors rather than mask them. I think that this implementation can be updated to include other error results once the previously described L0 issue is resolved. I can add a TODO comment for handling other errors here if you think it's appropriate.

[jandres742]

so it seems that if an error other than OUT_OF_DEVICE_MEMORY is returned, we need to mask it with SUCCESS, because we dont handle those errors. Could you elaborate on what cases you are seeing the error?

The matrix tests failing on Intel Arc are due to an unhandled error result. The call to zeContextMakeMemoryResident returns ZE_RESULT_ERROR_INVALID_ARGUMENT. This seems like a bug in L0 since zeContextMakeMemoryResident doesn't specify this value as a possible result. The argument values are not null so I'm not sure if there's anything we can do to handle this error.

I would say it is better we handle correctly those cases rather than masking the error with SUCCESS.

I agree. The current implementation masks all errors with success. The change I'm suggesting in this PR is to forward OOM errors rather than mask them. I think that this implementation can be updated to include other error results once the previously described L0 issue is resolved. I can add a TODO comment for handling other errors here if you think it's appropriate.

Thanks @0x12CC . I see it now. Ok, cool, please add a TODO, linking also the spec issue I just opened in the L0 spec so we fix that also there:

oneapi-src/level-zero-spec#240

[0x12CC]

@jandres742, I think this PR is ready to merge. I've added the requested TODOs and all tests are passing in intel/llvm#11696 except for the following:

********************
Failed Tests (1):
  SYCL :: Plugin/cuda-max-local-mem-size.cpp

This failure looks related to be53fb3 since it's in the CUDA adapter and the tests were passing before I merged the latest changes from the adapters branch.

[jandres742]

@jandres742, I think this PR is ready to merge. I've added the requested TODOs and all tests are passing in intel/llvm#11696 except for the following:

********************
Failed Tests (1):
  SYCL :: Plugin/cuda-max-local-mem-size.cpp

This failure looks related to be53fb3 since it's in the CUDA adapter and the tests were passing before I merged the latest changes from the adapters branch.

thanks @0x12CC .

@kbenzie : please merge when possible.

[kbenzie]

@jandres742, I think this PR is ready to merge. I've added the requested TODOs and all tests are passing in intel/llvm#11696 except for the following:

********************
Failed Tests (1):
  SYCL :: Plugin/cuda-max-local-mem-size.cpp

This failure looks related to be53fb3 since it's in the CUDA adapter and the tests were passing before I merged the latest changes from the adapters branch.

thanks @0x12CC .

@kbenzie : please merge when possible.

There's a failure on intel/llvm#11696 for the CUDA E2E tests. I suspect this might be due to that PR not containing the latest changes from the sycl branch. @0x12CC coudl you update that PR to see if we can get it passing before merging?

[0x12CC]

@jandres742, I think this PR is ready to merge. I've added the requested TODOs and all tests are passing in intel/llvm#11696 except for the following:

********************
Failed Tests (1):
  SYCL :: Plugin/cuda-max-local-mem-size.cpp

This failure looks related to be53fb3 since it's in the CUDA adapter and the tests were passing before I merged the latest changes from the adapters branch.

thanks @0x12CC .
@kbenzie : please merge when possible.

There's a failure on intel/llvm#11696 for the CUDA E2E tests. I suspect this might be due to that PR not containing the latest changes from the sycl branch. @0x12CC coudl you update that PR to see if we can get it passing before merging?

This failure is not fixed on the latest sycl branch. I believe it's caused by be53fb3 and is not related to these L0 changes.

[kbenzie]

This failure is not fixed on the latest sycl branch.

Incorrect, intel/llvm#11454 fixed this yesterday. I'm also not going to merged this, as per the Adapter Change Process, until I see the intel/llvm checks all pass so please update that PR.

[0x12CC]

This failure is not fixed on the latest sycl branch.

Incorrect, intel/llvm#11454 fixed this yesterday. I'm also not going to merged this, as per the Adapter Change Process, until I see the intel/llvm checks all pass so please update that PR.

Sorry for the confusion. I missed these commits in my last sync. I'm re-running the tests now with the sycl branch at intel/llvm@aa0171d.

[kbenzie]

Thank you @0x12CC

[0x12CC]

@kbenzie, all of the tests are now passing in intel/llvm#11696. I think this PR is ready to merge.

[kbenzie]

I've opened intel/llvm#11811 combing this with #1033 and #1028 to merge into intel/llvm at the same time to accelerate merging.