[SYCL][ESIMD] Ignore warning messages from CM_EMU for esimd_emulator #626

dongkyunahn-intel · 2021-12-16T19:40:23Z

Warning messages from CM_EMU library for esimd_emulator cause
failures for 'CHECK' and 'diff' commands
'XFAIL:esimd_emulator' is marked for tests using 'CHECK' command
'grep' command is inserted in pipeline for 'sycl-ls.cpp' test in
order to filter out those warning messages

- Warning messages from CM_EMU library for esimd_emulator cause failures for 'CHECK' and 'diff' commands - 'XFAIL:esimd_emulator' is marked for tests using 'CHECK' command - 'grep' command is inserted in pipeline for 'sycl-ls.cpp' test in order to filter out those warning messages

vzakhari · 2021-12-16T20:06:38Z

SYCL/DeviceLib/built-ins/printf.cpp

@@ -1,5 +1,11 @@
 // UNSUPPORTED: cuda || hip
 // CUDA and HIP don't support printf.
+
+// esimd_emulator : Warning messages from CM_EMU library for emulation


@kbobrovs do you think these comments need to be guarded with the ESIMD Intel feature?

@vzakhari - no, I don't think it needs to, as this is public feature. But all non-ESIMD tests should be guarded with // UNSUPPORTED: esimd_emulator, as this BE can only run ESIMD tests.

So, @dongkyunahn-intel, why bother adding // XFAIL: esimd_emulator rather than // UNSUPPORTED: esimd_emulator? Do you expect this test to pass in future?

'XFAIL' is already added below - line 7.

Please re-read my question. XFAIL should not be there.

smaslov-intel

LGTM

SYCL/Plugin/sycl-ls.cpp

… esimd_emulator_ignore_cm_warnings

vzakhari

DeviceLib LGTM

bader

Warning messages from CM_EMU library for esimd_emulator cause
failures for 'CHECK' and 'diff' commands

@dongkyunahn-intel, could you elaborate on the problem, please?
I'm not sure what scenarios are impacted. It looks like warnings appears for the test not using ESIMD feature, which means that it might impact any new test we add. If so, I don't think that adding "UNSUPPORTED: esimd_emulator" is the solution we want here.

dongkyunahn-intel · 2021-12-22T08:42:20Z

Warning messages from CM_EMU library for esimd_emulator cause
failures for 'CHECK' and 'diff' commands

@dongkyunahn-intel, could you elaborate on the problem, please? I'm not sure what scenarios are impacted. It looks like warnings appears for the test not using ESIMD feature, which means that it might impact any new test we add. If so, I don't think that adding "UNSUPPORTED: esimd_emulator" is the solution we want here.

Failure caused warning message is discussed here - #582 (comment).

'UNSUPPORTED' marking was applied for other non-ESIMD tests failing with 'esimd_emulator' per Konst's suggestion - 4ab7155.

bader · 2021-12-23T14:13:36Z

Failure caused warning message is discussed here - #582 (comment).

From the comment: "CM_EMU library supporting ESIMD_EMULATOR could print out extra info for debugging - like first three lines of above."
What does "could print" mean?
It looks like this PR updates half of all existing tests in the suite.
Imagine I'm adding a new test. How do I know if this message is printed and I need put an extra line to disable the test?
This sounds very inconvenient for test developers.

I suggest we treat cm emulator the same way to treat other external dependencies (e.g. OpenCL drivers). We have documented versions of these dependencies, which we tested and know won't cause too much troubles to our users (see https://github.com/intel/llvm/blob/sycl/buildbot/dependency.conf). We regularly test new versions and update this document if new versions pass testing and block updates if new version regress testing.
This issue sound like to be critical enough to block the update of cm emulator and wait for a fixed version.
Can we do that?

… esimd_emulator_ignore_cm_warnings

dongkyunahn-intel · 2022-01-11T07:26:04Z

From the comment: "CM_EMU library supporting ESIMD_EMULATOR could print out extra info for debugging - like first three lines of above." What does "could print" mean?

CM_EMU prints out various messages depending on environment variable setup like CM_RT_PLATFORM. What I tried to do with this PR was to ignore failures from those messages. There is on-going work for this log message suppression in CM_EMU side. Until CM_EMU is updated for this suppression, we need to ignore failures cause by those messages.

It looks like this PR updates half of all existing tests in the suite. Imagine I'm adding a new test. How do I know if this message is printed and I need put an extra line to disable the test? This sounds very inconvenient for test developers.

I think what you mean by 'half of all existing tests' is UNSUPPORTED marking. ESIMD_EMULATOR backend is supposed to be used only for kernels written for ESIMD. Because of this intention, other tests written for Non-ESIMD could fail because of features unsupported from ESIMD - like group. In order to filter out failures like this while merging some changes in intel/llvm, UNSUPPORTED marking needs to be inserted to many failing ones. My plan was to create another PR for this marking after this PR in order to clarify intention of each PR. However, Konst suggested applying this marking like my mention above.

bader · 2022-01-11T10:37:00Z

Maybe it would be better to add specific run command to run the test on ESIMD device (HW or emulator) like it's done for FPGA.
For example:

// RUN: %ESIMD_RUN_PLACEHOLDER %t.out &> %t.txt || true

This allows us to explicitly opt-in tests.
BTW, IIRC, all ESIMD tests are located in separate directory, so another option is configure LIT runner to use ESIMD emulator only for the tests in this directory.

dongkyunahn-intel · 2022-01-12T00:14:44Z

// RUN: %ESIMD_RUN_PLACEHOLDER %t.out &> %t.txt || true

Should the line above be added to all tests? Or only ESIMD tests?

@kbobrovs , what do you think of @bader 's suggestion?

kbobrovs · 2022-01-12T00:24:33Z

@bader, yes, we thought of such way of running tests. But I think this discords with overall lit testing approach, where device to run on is selected at the time of running the tests. All ESIMD test should run on both GPU and host emulation, and it is quite convenient to be able to run on host emulation only when Intel GPU device is not available. On the other hand, only ESIMD tests can run on host emulation, and we have to mark all other ones as UNSUPPORTED on non-emulator devices (or REQUIRE emulator device). Not sure what's best solution here.
Adding @vladimirlaz for possible comments.

bader · 2022-01-12T10:29:42Z

I'm strongly against forcing additional markings for non-ESIMD tests. These must be applied by default implicitly.

dongkyunahn-intel · 2022-01-12T20:52:05Z

@kbobrovs , can I merge this PR only for ignoring failures from CM_EMU warning logs after reverting changes for 'UNSUPPORTED' marking? My intel/llvm PR (intel/llvm#5058) is blocked because of warning messages - not because of lack of 'UNSUPPORTED' marking.

I would like to have discussion on this marking in another PR.

lsatanov · 2022-01-13T16:02:15Z

@kbobrovs , can I merge this PR only for ignoring failures from CM_EMU warning logs after reverting changes for 'UNSUPPORTED' marking? My intel/llvm PR (intel/llvm#5058) is blocked because of warning messages - not because of lack of 'UNSUPPORTED' marking.

I would like to have discussion on this marking in another PR.

Hi. As of recent, warnings are not displayed by default in EMU, this should not be a problem anymore.

dongkyunahn-intel · 2022-01-13T16:31:58Z

Hi. As of recent, warnings are not displayed by default in EMU, this should not be a problem anymore.

This change has not been propagated to open-source CM_EMU git repo where ESIMD_EMULATOR building process downloads source file from (https://github.com/intel/cm-cpu-emulation). I'm waiting for it.

kbobrovs · 2022-01-13T18:24:48Z

@kbobrovs , can I merge this PR

@dongkyunahn-intel, no, I don't think so, as @bader objects. We need some other solution

I would like to have discussion on this marking in another PR.

Are you going to close this one then? I'm little lost WRT to plans on this and other ESIMD emulator PRs.

dongkyunahn-intel · 2022-01-15T02:37:48Z

Closing this PR as warning message suppression from CM_EMU is applied in intel/llvm#5058.

dongkyunahn-intel requested a review from vladimirlaz December 16, 2021 19:40

dongkyunahn-intel requested review from smaslov-intel, vzakhari and a team as code owners December 16, 2021 19:40

vzakhari reviewed Dec 16, 2021

View reviewed changes

smaslov-intel previously approved these changes Dec 16, 2021

View reviewed changes

dongkyunahn-intel added 2 commits December 16, 2021 15:00

Reformatting 'TODO' comments not to be read as feature markup

607bbdc

'XFAIL:' to 'XFAIL' not to be read as feature markup

5e5554e

dongkyunahn-intel dismissed smaslov-intel’s stale review via 607bbdc December 16, 2021 23:11

dongkyunahn-intel requested review from kbobrovs and smaslov-intel December 16, 2021 23:13

Marking 'printf.cpp' as UNSUPPORTED

80039e6

vladimirlaz requested a review from vzakhari December 21, 2021 05:34

vladimirlaz previously approved these changes Dec 21, 2021

View reviewed changes

Marking 'srgba-read.cpp' as UNSUPPORTED for esimd_emulator

f923fa8

dongkyunahn-intel dismissed vladimirlaz’s stale review via f923fa8 December 21, 2021 17:48

kbobrovs suggested changes Dec 21, 2021

View reviewed changes

SYCL/Plugin/sycl-ls.cpp Outdated Show resolved Hide resolved

SYCL/Plugin/sycl-ls.cpp Show resolved Hide resolved

dongkyunahn-intel added 2 commits December 21, 2021 12:27

Marking 'UNSUPPORTED' for failing Non-ESIMD kernels

4ab7155

Merge branch 'intel' of https://github.com/intel/llvm-test-suite into…

3cd975c

… esimd_emulator_ignore_cm_warnings

dongkyunahn-intel requested review from againull, AGindinson, AlexeySachkov, bader, cperkinsintel, Fznamznon, jbrodman, MrSidims, Pennycook and rdeodhar as code owners December 21, 2021 20:35

dongkyunahn-intel requested review from romanovvlad, sergey-semenov and v-klochkov as code owners December 21, 2021 20:35

clang-format error fix

01d4ff1

vzakhari previously approved these changes Dec 21, 2021

View reviewed changes

bader suggested changes Dec 22, 2021

View reviewed changes

Merge branch 'intel' of https://github.com/intel/llvm-test-suite into…

a9f1c67

… esimd_emulator_ignore_cm_warnings

dongkyunahn-intel dismissed vzakhari’s stale review via a9f1c67 January 11, 2022 01:45

dongkyunahn-intel requested a review from a team as a code owner January 11, 2022 01:45

Merge branch 'intel' of https://github.com/intel/llvm-test-suite into…

097d8f1

… esimd_emulator_ignore_cm_warnings

Merge branch 'intel' into esimd_emulator_ignore_cm_warnings

2318025

dongkyunahn-intel closed this Jan 15, 2022

[SYCL][ESIMD] Ignore warning messages from CM_EMU for esimd_emulator #626

[SYCL][ESIMD] Ignore warning messages from CM_EMU for esimd_emulator #626

Uh oh!

Conversation

dongkyunahn-intel commented Dec 16, 2021

Uh oh!

vzakhari Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

dongkyunahn-intel Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vzakhari left a comment

Choose a reason for hiding this comment

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

dongkyunahn-intel commented Dec 22, 2021

Uh oh!

bader commented Dec 23, 2021

Uh oh!

dongkyunahn-intel commented Jan 11, 2022

Uh oh!

bader commented Jan 11, 2022

Uh oh!

dongkyunahn-intel commented Jan 12, 2022

Uh oh!

kbobrovs commented Jan 12, 2022

Uh oh!

bader commented Jan 12, 2022

Uh oh!

dongkyunahn-intel commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsatanov commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongkyunahn-intel commented Jan 13, 2022

Uh oh!

kbobrovs commented Jan 13, 2022

Uh oh!

dongkyunahn-intel commented Jan 15, 2022

Uh oh!

Uh oh!

dongkyunahn-intel commented Jan 12, 2022 •

edited

Loading

lsatanov commented Jan 13, 2022 •

edited

Loading