Recommit [CIR][Pipeline] Support -fclangir-analysis-only #832

ChuanqiXu9 · 2024-09-12T07:26:07Z

Reland #638

This was reverted due to #655. I tried to address the problem in the newest commit.

The changes of the PR since the last landed one includes:

Move the definition of cir::CIRGenConsumer to clang/include/clang/CIRFrontendAction/CIRGenConsumer.h, and leave its HandleTranslationUnit interface is left empty. So that cir::CIRGenConsumer won't need to depend on CodeGen any more.
Change the old definition of cir::CIRGenConsumer in clang/lib/CIR/FrontendAction/CIRGenAction.cpp and to CIRLoweringConsumer, inherited from cir::CIRGenConsumer, which implements the original HandleTranslationUnit interface.

I feel this may improve the readability more even without my original patch.

ChuanqiXu9 · 2024-09-12T09:02:28Z

I didn't understand why the CI fails on windows. The log says:

FAILED: tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenAction.cpp.obj 
sccache C:\PROGRA~2\MICROS~2\2019\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\a\clangir\clangir\build\tools\clang\lib\CodeGen -ID:\a\clangir\clangir\clang\lib\CodeGen -ID:\a\clangir\clangir\clang\include -ID:\a\clangir\clangir\build\tools\clang\include -ID:\a\clangir\clangir\build\include -ID:\a\clangir\clangir\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /O2 /Ob2  -std:c++17 -MD  /EHs-c- /GR- -UNDEBUG /showIncludes /Fotools\clang\lib\CodeGen\CMakeFiles\obj.clangCodeGen.dir\CodeGenAction.cpp.obj /Fdtools\clang\lib\CodeGen\CMakeFiles\obj.clangCodeGen.dir\ /FS -c D:\a\clangir\clangir\clang\lib\CodeGen\CodeGenAction.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3089): error C2027: use of undefined type 'mlir::MLIRContext'
D:\a\clangir\clangir\clang\include\clang/CIRFrontendAction/CIRGenAction.h(21): note: see declaration of 'mlir::MLIRContext'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3088): note: while compiling class template member function 'void std::default_delete<mlir::MLIRContext>::operator ()(_Ty *) noexcept const'
        with
        [
            _Ty=mlir::MLIRContext
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3198): note: see reference to function template instantiation 'void std::default_delete<mlir::MLIRContext>::operator ()(_Ty *) noexcept const' being compiled
        with
        [
            _Ty=mlir::MLIRContext
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3125): note: see reference to class template instantiation 'std::default_delete<mlir::MLIRContext>' being compiled
D:\a\clangir\clangir\clang\include\clang/CIR/CIRGenerator.h(72): note: see reference to class template instantiation 'std::unique_ptr<mlir::MLIRContext,std::default_delete<mlir::MLIRContext>>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3089): error C2338: can't delete an incomplete type
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\memory(3090): warning C4150: deletion of pointer to incomplete type 'mlir::MLIRContext'; no destructor called
D:\a\clangir\clangir\clang\include\clang/CIRFrontendAction/CIRGenAction.h(21): note: see declaration of 'mlir::MLIRContext'

but I didn't touch MLIRContext and the definition of mlir::MLIRContext should be visible in the CodeGenAction.cpp. @lanza @bcardosolopes would you like to take a look?

bcardosolopes · 2024-09-12T23:52:14Z

@ChuanqiXu9 I haven't seen this one before, it's not even related to ClangIR, very weird. Maybe it was a fluke? Try update the PR with something that triggers testing again, let's see if it reproduces

Close llvm#633. See the issue for details.

ChuanqiXu9 · 2024-09-13T08:52:00Z

@ChuanqiXu9 I haven't seen this one before, it's not even related to ClangIR, very weird. Maybe it was a fluke? Try update the PR with something that triggers testing again, let's see if it reproduces

Maybe some MSVC implementation details.. I solved it by adding the MLIR include dir in CodeGen conditionally. Now this is ready to review.

bcardosolopes · 2024-09-13T17:27:27Z

clang/lib/CodeGen/CodeGenAction.cpp

+
+#if CLANG_ENABLE_CIR
+  if (CI.getFrontendOpts().ClangIRAnalysisOnly)
+    AdditionalConsumers.push_back(cir::createCIRAnalysisOnlyConsumer(CI));


I'm a bit uncomfortable with touching anything under clang/lib/CodeGen, this is going to be a hassle to upstream / convince maintainers. Can't this be done instead in the opposite way? In CIRGenAction.cpp we could dispatch the classic CodeGen if the pipeline is asking for LLVM codegen and -fclangir-analysis-only is ON. Have you tried that and found any issue?

I'm a bit uncomfortable with touching anything under clang/lib/CodeGen

This is the style we did for modules. It shows it is pretty stable.

Can't this be done instead in the opposite way?

But if you really want, we can do it. We just need to introduce more lines of code. Please see the newest commit.

This is the style we did for modules. It shows it is pretty stable.

The approach is sound, my point is more about trying to minimize clang changes outside CIR dirs, so for the time being is less invasive to the rest of the codebase,

But if you really want, we can do it. We just need to introduce more lines of code. Please see the newest commit.

This looks great, thanks!

bcardosolopes

Thanks for the updates! Looks mostly good, one question remaining (and couple nits to address)

clang/include/clang/CIRFrontendAction/CIRGenAction.h

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

clang/lib/Frontend/CompilerInvocation.cpp

bcardosolopes · 2024-09-16T17:26:57Z

The bot failure is unrelated (#829), please ignore it

bcardosolopes

LGTM

Reland llvm#638 This was reverted due to llvm#655. I tried to address the problem in the newest commit. The changes of the PR since the last landed one includes: - Move the definition of `cir::CIRGenConsumer` to `clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its `HandleTranslationUnit` interface is left empty. So that `cir::CIRGenConsumer` won't need to depend on CodeGen any more. - Change the old definition of `cir::CIRGenConsumer` in `clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to `CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which implements the original `HandleTranslationUnit` interface. I feel this may improve the readability more even without my original patch.

Reland #638 This was reverted due to #655. I tried to address the problem in the newest commit. The changes of the PR since the last landed one includes: - Move the definition of `cir::CIRGenConsumer` to `clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its `HandleTranslationUnit` interface is left empty. So that `cir::CIRGenConsumer` won't need to depend on CodeGen any more. - Change the old definition of `cir::CIRGenConsumer` in `clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to `CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which implements the original `HandleTranslationUnit` interface. I feel this may improve the readability more even without my original patch.

ChuanqiXu9 requested review from lanza and bcardosolopes as code owners September 12, 2024 07:26

ChuanqiXu9 force-pushed the AnalysisOnly branch from 7dd9b4a to 242be05 Compare September 12, 2024 08:36

ChuanqiXu9 added 5 commits September 13, 2024 10:06

[CIR] [Pipeline] Support -fclangir-analysis-only

c38555f

Close llvm#633. See the issue for details.

Add dependency to CodeGen

5c42ee9

address comments

0bcce72

Define CLANG_ENABLE_CIR before consuming it

fec0716

Address comments

470e8b8

ChuanqiXu9 force-pushed the AnalysisOnly branch 3 times, most recently from b54c3b4 to 75edb18 Compare September 13, 2024 03:35

bcardosolopes reviewed Sep 13, 2024

View reviewed changes

Introduce cir::AnalysisOnlyAndEmit*Action

0237670

ChuanqiXu9 force-pushed the AnalysisOnly branch from 970cfa8 to 0237670 Compare September 14, 2024 03:41

update

63944ae

bcardosolopes requested changes Sep 16, 2024

View reviewed changes

clang/include/clang/CIRFrontendAction/CIRGenAction.h Show resolved Hide resolved

clang/lib/CIR/FrontendAction/CIRGenAction.cpp Show resolved Hide resolved

clang/lib/Frontend/CompilerInvocation.cpp Show resolved Hide resolved

address comments

56f17e4

bcardosolopes approved these changes Sep 18, 2024

View reviewed changes

bcardosolopes merged commit e437114 into llvm:main Sep 18, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recommit [CIR][Pipeline] Support -fclangir-analysis-only #832

Recommit [CIR][Pipeline] Support -fclangir-analysis-only #832

ChuanqiXu9 commented Sep 12, 2024 •

edited

Loading

ChuanqiXu9 commented Sep 12, 2024

bcardosolopes commented Sep 12, 2024

ChuanqiXu9 commented Sep 13, 2024

bcardosolopes Sep 13, 2024

ChuanqiXu9 Sep 14, 2024 •

edited

Loading

bcardosolopes Sep 16, 2024 •

edited

Loading

bcardosolopes left a comment

bcardosolopes commented Sep 16, 2024

bcardosolopes left a comment

Recommit [CIR][Pipeline] Support -fclangir-analysis-only #832

Recommit [CIR][Pipeline] Support -fclangir-analysis-only #832

Conversation

ChuanqiXu9 commented Sep 12, 2024 • edited Loading

ChuanqiXu9 commented Sep 12, 2024

bcardosolopes commented Sep 12, 2024

ChuanqiXu9 commented Sep 13, 2024

bcardosolopes Sep 13, 2024

Choose a reason for hiding this comment

ChuanqiXu9 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

bcardosolopes Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

bcardosolopes left a comment

Choose a reason for hiding this comment

bcardosolopes commented Sep 16, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

ChuanqiXu9 commented Sep 12, 2024 •

edited

Loading

ChuanqiXu9 Sep 14, 2024 •

edited

Loading

bcardosolopes Sep 16, 2024 •

edited

Loading