[PATCH] D122082: Add DXIL Bitcode Writer and DXIL testing

Mon Mar 21 15:03:47 PDT 2022

nikic added a comment.

In D122082#3395250 <https://reviews.llvm.org/D122082#3395250>, @beanz wrote:

> In D122082#3394836 <https://reviews.llvm.org/D122082#3394836>, @nikic wrote:
>
>> Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.
>
> Not to be difficult, but was this discussed somewhere that I missed? I didn’t see it in any of the status updates or project plans for opaque pointers. I have no objection to doing the work (I’ve already started). I actually don’t even mind not merging this patch until I have it working. The opaque pointer effort will actually be a net win for the larger project I’m working on, so kudos to that :D.
>
> I greatly dislike “you can’t land <x> because of <not done thing>” being a requirement _ever_ in LLVM, but especially in cases where the not completed work doesn’t have a final completion target (as the latest status update on opaque pointers says).
>
> We held up a lot of great work behind the “new” pass manager. For _years_ nobody was allowed to add features to the “legacy” pass manager or even fix infrastructure issues in ways that relied on the pass manager.
>
> Drawing hard lines in the sand like this is fine if there is wide community acceptance (as with any coding standard), but nobody should be able to draw these lines without discussion and agreement.

I'm not sure what you want to hear here. The opaque pointers migration was decided many years ago, long before I started working on LLVM. Well-informed reviewers have been rejecting patches violating opaque pointers principles years before LLVM even had a proper opaque pointer type. Of course, enforcement on this has been inconsistent and somewhat dependent on which reviewer you hit.

Now that the opaque pointers migration is close to complete I'm keeping an active eye out to make sure there are no regressions. LLVM itself is essentially clean under opaque pointers, and we're maybe two weeks away from proposing a default switch in Clang. Which is why I'm so surprised that literally the first time I'm seeing push-back against an opaque pointer requirement in a review is //now//.

I think that for this change in particular it is important to resolve this issue in advance, because it may impact your overall design in a non-trivial way. Your current handling of DXIL is predicated on LLVM IR and DXIL being essentially the same, minus a few cosmetic differences like fneg vs fsub. With opaque pointers, LLVM IR and DXIL will differ in a more substantial way, and I'd naively expect this to have some impact on your implementation approach.

Finally, if you want to make a new pass manager analogy, I'm not telling you that you can't work on the legacy pass manager, I'm only telling you that you should implement functionality for both the legacy and new pass manager at the same time. That seems like a pretty reasonable request to me? In fact, our biggest failures in the NewPM migration were related to cases where this principle was not followed, and changes were made only to either the new or the legacy pass manager. Let's avoid that to the degree it is possible.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122082/new/

https://reviews.llvm.org/D122082