This is an archive of the discontinued LLVM Phabricator instance.

headers: optionalise some generated resource headers
ClosedPublic

Authored by compnerd on Oct 31 2021, 11:34 AM.

Details

Summary

This splits out the generated headers and conditonalises them upon the
target being enabled.

The motivation here is that the RISCV header alone added 10MB to the
resource directory, which was previously at 10MB, increasing the build
size and time. This header is contributing ~50% of the size of the
resource headers (~10MB).

The ARM generated headers are contributing about ~10% or 1MB.

This could be extended further adding only the static resource headers
for the targets that the LLVM build supports.

Diff Detail

Event Timeline

compnerd created this revision.Oct 31 2021, 11:34 AM
compnerd requested review of this revision.Oct 31 2021, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2021, 11:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This makes sense to me. It doesn't make sense to spend the time generating files that will never get used, nor spend the space for storing them.
riscv_vector.h in particular is quite large, coming in at just under 10M, and there are two of them showing up in my build at tools/clang/lib/Headers/riscv_vector.h and lib/clang/10.0.0/include/riscv_vector.h.

I am undecided between this may break something and I cannot use the risc-v headers on an x86 machine.

compnerd added a comment.EditedOct 31 2021, 2:07 PM

I am undecided between this may break something and I cannot use the risc-v headers on an x86 machine.

This fundamentally cannot break something outside of the RISC-V target, it is applicable to any and all hosts. However, the condition ensures that RISC-V support is not enabled when building the compiler.

#ifndef __riscv_vector
#error "Vector intrinsics require the vector extension."
#endif

__riscv_vector is in the implementation's namespace, so the user may not define this (it is defined by the compiler when targeting a RISCV core with the V extension). Trying to use -target riscv32-unknown-*-* or -target riscv64-unknown-*-* (not literally, but with a concrete value), will fail due to RISCV support being disabled in the build. That is, the header cannot be used on any machine when not targeting RISCV. Because RISCV is not enabled, it is safe to elide the header.

However, given your response, I think that it might be a good idea to elaborate this in commit message. Could you help me identify what information would be valuable to add to the commit message?

If I understand you correctly, I would need to pass something ala -target riscv-xx to enable __riscv_vector. However, this is impossible because the risk target is disabled. So I thing it is save what you are doing.

Right, the header is only necessary when you're targeting riscv, so if compiling for riscv isn't supported by the compiler noted in the LLVM_TARGETS_TO_BUILD, you don't need it and can't use it anyway.
This is separate from the host platform that the compiler is running on.

Right, the header is only necessary when you're targeting riscv, so if compiling for riscv isn't supported by the compiler noted in the LLVM_TARGETS_TO_BUILD, you don't need it and can't use it anyway.
This is separate from the host platform that the compiler is running on.

I would instead say that the header is only necessary and available when you're targeting riscv. If you are targeting ARM, the header is neither available nor useful.

If I understand you correctly, I would need to pass something ala -target riscv-xx to enable __riscv_vector. However, this is impossible because the risk target is disabled. So I thing it is save what you are doing.

Correct. You would need to invoke clang as clang -target riscv64-unknown-linux-musl -march=rv64gv0p10 -menable-experimental-extensions as a concrete example. At that point, you would be able to include riscv_vector.h for the declarations.

rnk added a comment.Nov 1 2021, 10:44 AM

I think this seems reasonable.

Generally speaking, we have tests in clang that generate IR for targets that are not enabled in LLVM. It is kind of nice: you don't have to mark the IRGen tests with REQUIRES: foo-registered-target. If we want to test these ARM and RISCV intrinsic tests in clang, now, they will have to be conditioned on the relevant target being enabled. I think that's fine.

This revision is now accepted and ready to land.Nov 8 2021, 10:36 AM
This revision was automatically updated to reflect the committed changes.

I remember that NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py needs to be the first line...

But thanks for the work: people not enabling RISCV should not incur so much overhead. I hope that RISC-V vector contributors can really bear the cost in mind.

jrtc27 added a comment.Nov 9 2021, 2:48 PM

This change seems pretty counter to Clang's ability to spit out IR for anything regardless of what backends you have. The right fix should be to shrink the headers, not mask the problem by changing the principles of how Clang works.

jrtc27 added a comment.Nov 9 2021, 2:53 PM

And I don't see any AArch64 or ARM maintainers ACKing this. You haven't even tagged any as reviewers.

thakis added a subscriber: thakis.Dec 13 2021, 4:02 AM

This change seems pretty counter to Clang's ability to spit out IR for anything regardless of what backends you have. The right fix should be to shrink the headers, not mask the problem by changing the principles of how Clang works.

For what it's worth, I agree with this. "RISC-V did it already" seems like a weak justification given relative popularity of RISC-V and ARM.

What's more, I haven't seen the intrinsic header generation take up a lot of time, and 10 MB isn't a lot of storage.

(Intrinsic _tests_ on the other hand did take quite a lot of time on RISC-V, which is why they got disabled if the RISC-V is off. This doesn't help bots much, since bots need to enable all backends. We had a discussion about this when the intrinsics got added, but in the end we sadly just accepted a big increase in test time instead of figuring out some less expensive approach.)

Anyways, I agree with @jrtc27 about this change.

(…at least for the arm headers, which

  1. didn't use to do this
  2. are a lot smaller than the risc-v headers

)