[SYCL][ESIMD][NewPM] Add ESIMDVerifierPass to new PM pipeline in CodeGen #5298
Conversation
Signed-off-by: Mikhail Lychkov <[email protected]>
|
The full set of fixes for new PM also validated in #4860. |
| @@ -1385,6 +1385,12 @@ void EmitAssemblyHelper::RunOptimizationPipeline( | |||
| // configure the pipeline. | |||
| OptimizationLevel Level = mapToLevel(CodeGenOpts); | |||
|
|
|||
| if (LangOpts.SYCLIsDevice) | |||
There was a problem hiding this comment.
ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.
@sndmitriev - what do you think?
There was a problem hiding this comment.
ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.
@sndmitriev - what do you think?
Please note that disable-llvm-passes disables even such passes as AlwaysInline, which are required to preserve the semantics of always_inline attribute, i.e. the flag literally says disable llvm passes, not optimization or transformation passes, but llvm passes.
My understanding is that the flag is not intended to be used by end users unless they know what they are doing and therefore, I would prefer to leave ESIMD verification disabled if -disable-llvm-passes is present.
@bader, what do you think?
There was a problem hiding this comment.
I agree with Alexey. -disable-llvm-passes is supposed to disable all LLVM passes with no exceptions.
I think what Konst has in mind is that ESIMD verifier pass should run even with -O0.
There was a problem hiding this comment.
ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.
Can we move the logic of this pass to FE Sema then?
There was a problem hiding this comment.
@kbobrovs @sndmitriev Could you please provide any comments?
There was a problem hiding this comment.
Sorry for delay.
Unfortunately, we can't move this to Sema as it was identified as very time-consuming task by the FE team, so we went with the simpler solution.
I think what Konst has in mind is that ESIMD verifier pass should run even with -O0.
Yes, exactly.
This patch doesn't disable |
Signed-off-by: Mikhail Lychkov <[email protected]>
mlychkov
dismissed stale reviews from elizabethandrews, smanna12, and AlexeySachkov
via
988c7d9
The test is updated to check that ESIMDVerifier pass is executed for O0 opt level as well. |
|
Looks like failure of HIP tests is caused by #5374. |
I don't think so. These tests were passing in #5374 pre-commit. It might be due to a conflict with some other changes or maybe tested sources didn't have #5374 changes. Could you update your branch to make sure that #5374 is included, please? |
|
Another one failure due to CI infra problems. Raised an issue and restarted failed job. |
Signed-off-by: Mikhail Lychkov [email protected]