[PATCH] D83477: [Matrix] Tighten LangRef definitions and Verifier checks.

Mon Jul 13 02:45:41 PDT 2020

fhahn added a comment.

In D83477#2146778 <https://reviews.llvm.org/D83477#2146778>, @SjoerdMeijer wrote:

> Had to revert this because somehow I missed a few failing regression test. Regarding this, wanted to check one thing @fhahn.
>
> In `test/Transforms/LowerMatrixIntrinsics/strided-store-i32.ll`, we have for example:
>
>   call void @llvm.matrix.column.major.store(<6 x i32> %in, i32* %out, ..
>   
>
> And I am thinking that this should be:
>
>   call void @llvm.matrix.column.major.store(<6 x i32> %in, <6 x i32>* %out, ..
>   
>
> This would match the intrinsic description which says that the second argument should be a pointer to the first matched type:
>
>   [llvm_anyvector_ty, LLVMAnyPointerType<LLVMMatchType<0>>,
>   
>
> Agree, or am I perhaps missing something? If not and you agree, I will modify the loads/store tests in test/Transforms/LowerMatrixIntrinsics/ before recommitting this.

I think the intrinsic definition is wrong here (and it also seems like the `LLVMAnyPointerType` does not actually result in the expected check). I think should pass a pointer to the element type directly (rather than a pointer to a vector), because if  `stride > R` we would access elements outside of the vector. Granted, nothing should really rely on the pointer type for aliasing purposes and so on, but it seems misleading to pass in e.g. `<6 x i32>*` and then access elements other than the first 6 `i32`, e.g. due to the stride being 10.

I missed that in the adjustments of the langref, I think we specify that %Ptr needs to be a pointer to the element type of the vector.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83477