[L0 V2] USM advice flags translation is incorrect

[Bensuo]

It appears as if the L0 V2 adapter is not correctly translating ur_usm_advice_flags_t to ze_memory_advice_t, as a modified example of this code from ur_command_list_manager::appendUSMAdvise()

  advice = UR_USM_ADVICE_FLAG_SET_READ_MOSTLY;
  auto zeAdvice = ur_cast<ze_memory_advice_t>(advice);

This cast results in a zeAdvice value of ZE_MEMORY_ADVICE_SET_PREFERRED_LOCATION because the UR type is defined as bitfield values and the ZE type is just an enum with incremental values. There are also more UR values for memory advice than in L0, so this straight cast can result in invalid values for options which have no mapping to L0.

The V1 adapter does not have this problem because it handles and translates the bitfield flags specifically:

llvm/unified-runtime/source/adapters/level_zero/command_buffer.cpp

Line 1367 in e6e1703

if (Advice & UR_USM_ADVICE_FLAG_SET_READ_MOSTLY)

[Bensuo]

FYI: @intel/unified-runtime-reviewers-level-zero

[igchor]

@Bensuo Are you sure the legacy implementation is correct? Since ze_memory_advice_t is only an enum it can only represent a single advice at any given moment, right?

I think we might need to just loop over all the set bits in the UR advice and for each, call a separate zeCommandListAppendMemAdvise with appropriate value.

[Bensuo]

@Bensuo Are you sure the legacy implementation is correct? Since ze_memory_advice_t is only an enum it can only represent a single advice at any given moment, right?

I think we might need to just loop over all the set bits in the UR advice and for each, call a separate zeCommandListAppendMemAdvise with appropriate value.

@igchor Yes I'm not sure that any implementation has ever been correct for passing multiple UR advice flags at once, the flags change was ~2 years ago from what I can tell. I think V1 (for command buffers anyway, normal usage seems to just be doing the same cast as V2) is at least correct for a single flag right now.

Do you think this is even worth correcting for V1? L0 says that it shouldn't affect functional correctness as they're just hints, though we did recently see failing tests from incorrect dependencies between advise and other commands that should be fixed by #19140 so that doesn't give me confidence that leaving it alone won't cause other problems.

[igchor]

I think it might be worth fixing. We need to change the code for v2 anyway, so we can make a fix and put the implementation in a common function. Looking at the other adapters, CUDA and HIP seem to handle this correctly:

llvm/unified-runtime/source/adapters/cuda/enqueue.cpp

Line 65 in 5dc9b12

for (auto &FlagPair : URToCUMemAdviseDeviceFlagsMap) {