[PATCH] D122270: Support converting pointers from opaque to typed

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

This approach looks good to me.

Depending on `bitcast` instructions inserted by the DXILPrepare pass feels slightly iffy. It's an assumption about the IR that is not covered by the standard "IR contract". Then again, it is quite common for the existing backends to have such assumptions, e.g. around legality in GlobalISel. So I think this is acceptable.

Making ValueEnumerator more generically useful makes a lot of sense to me, I really only have one concern about //how// that is done here, see the inline comment.



================
Comment at: llvm/lib/Bitcode/Writer/ValueEnumerator.cpp:365
+                                 bool ShouldPreserveUseListOrder,
+                                 Type *PrefixType)
     : ShouldPreserveUseListOrder(ShouldPreserveUseListOrder) {
----------------
Can you explain why PrefixType is needed? It seems like a strange wart in the API.

If this is really necessary, I feel like a cleaner API for the ValueEnumerator itself would be to split the enumeration out of the constructor, so that it's used as (not sure where ShouldPreserveUseListOrder should go):
```
ValueEnumerator VE;
VE.EnumerateModule(M, ShouldPreserveUseListOrder);
```
You could then inject an EnumerateType in the DirectX backend. There may have to be a `Finalize` method that must be called exactly once.

I haven't thought about how that change would propagate through the BitCodeModuleWriter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122270