tra added a comment. In D77777#1972720 <https://reviews.llvm.org/D77777#1972720>, @hliao wrote:
> In D77777#1972349 <https://reviews.llvm.org/D77777#1972349>, @tra wrote: > > > The patch could use a more detailed description. Specifically, it does not > > describe the purpose of these changes. > > > > > Replace them with the internal version, i.e. nvvm.texsurf.handle.internal > > > just before the instruction selector. > > > > It's not clear what is 'them'. 'nvvm.texsurf.handle' ? > > If so, do we need 'internal' any more? Can we just rename internal and be > > done with it? Adding an extra pass just to replace one intrinsic with > > another seems to be unnecessary. > > > > I may be missing something here. Why do we have internal and non-internal > > intrinsics at all? Do we need both? > > > besides required by NVVM IR spec, NVVM IR spec is for nvidia's own compiler. It's based on LLVM, but it does not impose direct constraints on LLVM's design choices. > the metadata in that intrinsic is a trick to prevent it from being sunk into > common code during optimization in LLVM IR. This sounds like it may have been done that way in an attempt to work around a problem with intrinsics' constraints. We may want to check if there's a better way to do it now. Right now both intrinsics are marked with `[IntrNoMem]` which may be the reason for compiler feeling free to move it around. We may need to give compiler correct information and then we may not need this just-in-time intrinsic replacement hack. I think it should be at least `IntrArgMemOnly` or, maybe `IntrInaccessibleMemOrArgMemOnly`. > NVPTX backend only handles the `internal` version. This is obviously fixable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77777/new/ https://reviews.llvm.org/D77777 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
