This is an archive of the discontinued LLVM Phabricator instance.

Restore CodeGen/LowLevelType from `Support`
ClosedPublic

Authored by chapuni on Apr 19 2023, 5:27 PM.

Details

Summary

This is rework of;

Since I have introduced llvm-min-tblgen as D146352, llvm-tblgen
may depend on CodeGen.

LowLevlType.h originally belonged to CodeGen. Almost all userse are
still under CodeGen or Target. I think CodeGen is the right place
to put LowLevelType.h.

MachineValueType.h may be moved as well. (later, D149024)

I have made many modules depend on CodeGen. It is consistent but
inefficient. It will be split out later, D148769

Besides, I had to isolate MVT and LLT in modmap, since
llvm::PredicateInfo clashes between TableGen/CodeGenSchedule.h
and Transforms/Utils/PredicateInfo.h.
(I think better to introduce namespace llvm::TableGen)

Depends on D145937, D146352, and D148768.

Diff Detail

Event Timeline

chapuni created this revision.Apr 19 2023, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 5:27 PM
chapuni requested review of this revision.Apr 19 2023, 5:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 19 2023, 5:27 PM
chapuni retitled this revision from Rename includes to Restore MVT and LLT into llvm/CodeGen.Apr 19 2023, 5:28 PM
chapuni edited the summary of this revision. (Show Details)
chapuni edited the summary of this revision. (Show Details)Apr 20 2023, 7:10 AM
chapuni updated this revision to Diff 516158.Apr 23 2023, 5:06 AM
chapuni edited the summary of this revision. (Show Details)
  • Update CMake deps
  • s/LowLevelType/LowLevelTypeUtils/
  • modmap: Prune LLVM_intrinsic_gen
chapuni updated this revision to Diff 516177.Apr 23 2023, 9:20 AM
chapuni retitled this revision from Restore MVT and LLT into llvm/CodeGen to Restore CodeGen/LowLevelType.
chapuni edited the summary of this revision. (Show Details)
  • Split out
arsenm accepted this revision.Apr 24 2023, 1:06 PM
This revision is now accepted and ready to land.Apr 24 2023, 1:06 PM

This is rework of;

D30046 (LLT)

Add some information why this is restored? Assume that people may not read the depended patches (Depends on D145937, D146352, and D148768.).

barannikov88 added a subscriber: barannikov88.EditedMay 1 2023, 10:07 PM

Adding CodeGen dependency to MCTargetDesc/AsmParser/Disassembler does not seem right. Why is it necessary?

Add some information why this is restored? Assume that people may not read the depended patches (Depends on D145937, D146352, and D148768.).

Yes sure. This was just an isolated diff with similar changes at first.

Adding CodeGen dependency to MCTargetDesc/AsmParser/Disassembler does not seem right. Why is it necessary?

This commit shows they may depend on CodeGen theoretically and actually (but not efficient)
I will decouple again in D148769.

chapuni updated this revision to Diff 518713.May 2 2023, 7:18 AM
chapuni retitled this revision from Restore CodeGen/LowLevelType to Restore CodeGen/LowLevelType from `Support`.
chapuni edited the summary of this revision. (Show Details)
  • Rebase and update the desc
This revision was landed with ongoing or failed builds.May 2 2023, 8:18 AM
This revision was automatically updated to reflect the committed changes.

Adding CodeGen dependency to MCTargetDesc/AsmParser/Disassembler does not seem right. Why is it necessary?

This commit shows they may depend on CodeGen theoretically and actually (but not efficient)
I will decouple again in D148769.

Sorry, I don't follow. They may theoretically depend on anything. Why CodeGen/CodeGenTypes, specifically?
If they don't require it, why add it?

Hi, this seems to have broken my bolt+debug+shared build. I don't think there are build bots for this configuration but you can reproduce it like this:

cmake -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
               -DLLVM_ENABLE_PROJECTS="clang;lld;bolt" \
               -DBUILD_SHARED_LIBS=True \
               -DLLVM_BUILD_TESTS=True \
               -DLLVM_CCACHE_BUILD=ON \
               -DLLVM_ENABLE_LLD=True \
               -DLLVM_TARGETS_TO_BUILD="X86;RISCV;AArch64" \
               -DLLVM_APPEND_VC_REV=False ../llvm

And this is the linker error:

ld.lld: error: undefined symbol: llvm::LLT::print(llvm::raw_ostream&) const
>>> referenced by LowLevelType.h:269 (/.../llvm-project/llvm/include/llvm/CodeGen/LowLevelType.h:269)
>>>               tools/bolt/lib/Passes/CMakeFiles/LLVMBOLTPasses.dir/AsmDump.cpp.o:(llvm::LLT::dump() const)
collect2: error: ld returned 1 exit status

Hi, this seems to have broken my bolt+debug+shared build. I don't think there are build bots for this configuration but you can reproduce it like this:

Same with flang+debug+shared build:

ld.lld: error: undefined symbol: llvm::LLT::print(llvm::raw_ostream&) const
>>> referenced by LowLevelType.h:269 (/llvm-project/llvm/include/llvm/CodeGen/LowLevelType.h:269)
>>>               CMakeFiles/obj.FIRTransforms.dir/SimplifyIntrinsics.cpp.o:(llvm::LLT::dump() const)

I can add CodeGenTypes link component in flang/lib/Optimizer/Transforms/CMakeLists.txt, but I am worried about the comment in llvm/lib/CodeGen/CMakeLists.txt:

# Be careful to append deps on this, since Targets' tablegens depend on this.
add_llvm_component_library(LLVMCodeGenTypes

I am not sure whether I need to be careful about adding dependencies onto LLVMCodeGenTypes (as I am planning to do) or about adding dependencies for LLVMCodeGenTypes target in llvm/lib/CodeGen/CMakeLists.txt :)

Sorry, I don't follow. They may theoretically depend on anything. Why CodeGen/CodeGenTypes, specifically?
If they don't require it, why add it?

I added deps pessimistically, "This depends on CodeGenTypes if LowLevelType.h is included".

Hi, this seems to have broken my bolt+debug+shared build. I don't think there are build bots for this configuration but you can reproduce it like this:

Sorry for inconvenience. Could you add CodeGenTypes in LINK_COMPONENTS please?

I can add CodeGenTypes link component in flang/lib/Optimizer/Transforms/CMakeLists.txt, but I am worried about the comment in llvm/lib/CodeGen/CMakeLists.txt:

# Be careful to append deps on this, since Targets' tablegens depend on this.
add_llvm_component_library(LLVMCodeGenTypes

I am not sure whether I need to be careful about adding dependencies onto LLVMCodeGenTypes (as I am planning to do) or about adding dependencies for LLVMCodeGenTypes target in llvm/lib/CodeGen/CMakeLists.txt :)

The latter. Excuse my wrong wording.

Sorry, I don't follow. They may theoretically depend on anything. Why CodeGen/CodeGenTypes, specifically?
If they don't require it, why add it?

I added deps pessimistically, "This depends on CodeGenTypes if LowLevelType.h is included".

Sorry if I was not being clear. Dependencies are harmful and should be avoided wherever possible.
They should not be added "just in case". Now when I want to build just llvm-mc I have to build half of the whole stack.
Although D148769 improves the situation, it does not justify adding these dependencies.
Your RFC was about dependencies, so I think you understand my concern.
Please try to remove them and only keep the ones that are really necessary.

Hi, this seems to have broken my bolt+debug+shared build. I don't think there are build bots for this configuration but you can reproduce it like this:

Sorry for inconvenience. Could you add CodeGenTypes in LINK_COMPONENTS please?

No worries. It feels a bit wrong though to have to add transitive dependencies to LINK_COMPONENTS.

My build is also fixed by moving the implementation of LLT::dump to a .cpp file. Would it be an option to apply the following patch?