[llvm] [NVPTX] Further refactor intrinsic definitions to remove redundancy (NFC) (PR #139924)

Alex MacLean via llvm-commits llvm-commits at lists.llvm.org
Wed May 14 12:59:22 PDT 2025


AlexMaclean wrote:

> Nice to see those tex/surf intrinsics combined into something less verbose. Setting intrinsic attributes via encompassing `let` is a mixed blessing. It's fine for smaller blocks, where one can see them.
> 
> They are a bit of a regression in terms of readability for larger blocks, as it makes it too easy to look at an intrinsic, and completely miss the attributes that are set somewhere else, out of view. Considering that those properties are critical for the intrinsics, it's not ideal.
> 
> Not sure whether it's something we need to fix -- large-block `let` already exist in other places, so we can certainly live with it, but it would be nice if we could keep relevant info easily visible where it matters. Perhaps we could apply `let` over parts of particularly large blocks. It will be redundant, but may work well enough for keeping things readable. WDYT?

Looking through the file, the only place I see where there seems like a problem is the type-conversion intrinsics. In all the other cases, it looks like the `let` fits pretty comfortably onto my screen. Since it seems like this is just a problem in this one place, I think it is probably fine to live with the large let. Alternately if you prefer, I could define a `PureIntrinsic` class which is a `DefaultAttrsIntrinsic` with `IntrSpeculatable` and `IntrNoMem`. I could then use this for all type-conversion intrinsics instead of a let. Would that work?

https://github.com/llvm/llvm-project/pull/139924


More information about the llvm-commits mailing list