[Mlir-commits] [mlir] [mlir][memref-to-spirv]: Reverse Image Load Coordinates (PR #160495)

[Jack Frankland]

FranklandJack wrote:

> > I may have misunderstood the SPIR-V/Vulkan specs here, so apologies if this isn't the case.
> 
> I am not a SPIR-V/Vulkan expert either, but I agree with your assessment. I guess it's a bit tricky since Image does not really have a layout, just coordinates.
> 
> > (although maybe this isn't a safe assumption if the layout isn't row-major 🤷 )
> 
> I think this is an important point, we only need to reverse indices for row-major `memref`s, and column-major should just work as it is. And I guess any other layout should not be supported - unless conversion can transform indices to match the logical Image coordinates.
> 
> So, my recommendation would be:
> 
>     1. Only do the reversal for row-major `memref`s.
> 
>     2. Update the description with what we discussed so the reason behind the decision is clear; also, I would use `i/u`, `j/v`, `k/w`, instead of `x`, `y`, `z`.
> 
>     3. minor: Update tests to use non-square/cube images; having all dimension the same in tests does not help the mental model.

Okay so, I started implementing these changes but thinking about this a bit more, does SPIR-V provide any mapping from texel coordinates to hardware memory, or are images and texels completely opaque? 

In the case of MLIR's memrefs we have a mapping at the IR level, namely the layout given by the memref's type which is assumed to be the identity if none is present (I guess a backend is free to remap the layout however it likes, but that is up to the target). In this conversion we are providing a generic lowering that is assuming for images width is the contiguous dimension in memory, followed by height and then depth. Whilst this might be a valid assumption for some hardware, is this true for all targets? Does this lowering as an upstream conversion even make sense? 

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