In D124804#3526834, @kuhar wrote:

In total, I counted ~1500 uses of ArrayRef<uint8_t> in the monorepo, so I don't consider it an uncommon pattern.

libObject uses MemoryBufferRefs which play back and forth with StringRef, and while it has been pointed out over and over again that many of StringRef’s methods should probably be on MemoryBufferRef because they are more broadly useful, it is the way it is.

LGTM.

Fixing incorrect code coment.

Adding @kuhar.

Two small nitpicks, otherwise LGTM.

LGTM.

Updating based on PR feedback. Thank's @kuhar!

Two super nitpick points off the top:
(1) Please be consistent that DXIL is all capitalized. I'd like LLVM to not have the annoyance issues that DXC's codebase has where DXIL is inconsistently capitalized.
(2) Can we instead name the pass "DXILTranslateMetadata"? The pass doesn't actually emit the metadata, it is translating it from one form in the module to another.

LGTM. Thank you for a nice improvement!

The other issue should be fixed in rGb26e44e623c75c084e865084b18541c6a1736df2.

Bots should be fixed with rG55e13a6bc0d6a31afc258a012184253041b0eb8e.

LGTM.

One more set of updates. Thanks @MaskRay!

Updates based on PR feedback

Updates based on PR feedback.

I posted a PR to swift that migrates build-script to use LLVM_ENABLE_RUNTIMES: https://github.com/apple/swift/pull/58465

LGTM! Thank you!

Making it an ERROR and providing an option to downgrade it to a WARNING seems reasonable to me. Thoughts?

Making the warning message more specific.

Posted an RFC: https://discourse.llvm.org/t/rfc-deprecate-and-remove-llvm-build-external-compiler-rt/62058

I pointed out a few (not all) the places where you have unneeded brackets. Also all your new files don't have newlines at the end of them (the C standard specifies that as a requirement although pretty much all compilers just issue a warning).

I don't want to roadblock or say "no we absolutely can't let this in", _but_... Apple Clang and Swift have long been the only reason we've kept this code alive. I didn't have the time to fix Apple Clang years ago, and nobody has made the effort since.

In D124474#3475539, @azharudd wrote:

In this case I'm just trying to make it work with existing build configurations where it is being used (for example Swift). We should definitely be moving away from LLVM_BUILD_EXTERNAL_COMPILER_RT in favor of LLVM_ENABLE_RUNTIMES eventually though.

I question whether we should be extending this or killing it off... Is there a reason you're using LLVM_BUILD_EXTERNAL_COMPILER_RT instead of LLVM_ENABLE_RUNTIMES=compiler-rt?

Rebasing and cleaning up code.

Fixing a code comment.

Updates based on PR feedback.

This should fix the patch upload, also added in some more of @kuhar's suggestions :).

Ugh... please hold off reviewing. I somehow messed up the commit update and got a lot of noisy diffs.

Updates based on review feedback. Thanks @kuhar!

Comments below. Updated patch coming shortly.

Overall this seems like a nice change. There is one unrelated change fixing a typo in here that should be removed and posted separately.

LGTM

This looks good to me and seems like a good added check.

I already merged a fix to resolve the test errors.

@wolfgangp I can't reproduce the failure locally, but I have a guess what's going wrong. I _think_ the issue is that the SmallStrings aren't null terminated and the cleared allocations aren't zero'd. I pushed a speculative fix in rG329abac134a3. Hopefully that fixes it. If not feel free to revert the changes, @python3kgae you'll have to diagnose further issues as I'll be afk all of next week.

@nhaehnle sorry, I realized this patch hadn't been rebased since the request to just fork the full bitcode writer.

Rebasing patches on the updated patch stack.

Rebasing on updated patches from earlier in the stack.

Updates to test cases to increase coverage.

One comment inline. Update coming in a few minutes.

Updating based on PR feedback.

LGTM