[PATCH] D136483: [mlir][MemRefToLLVM] Reuse existing lowering for collaspe/expand_shape

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added subscribers: cathyzhyi, springerm.
qcolombet added a comment.

BTW, regarding the dynamic case, I noticed that the new code and the old code differ.

Here is the ir for `collapse_shape_dynamic_with_non_identity_layout`, after renaming the variables and reordering them for clearer diff:

  --- old.ll      2022-11-05 06:19:12.140322531 +0000
  +++ new.ll      2022-11-05 06:22:09.989617895 +0000
  @@ -1,31 +1,23 @@
   ; ModuleID = '<stdin>'
   source_filename = "LLVMDialectModule"
   
   declare ptr @malloc(i64)
   
   declare void @free(ptr)
   
   define { ptr, ptr, i64, [2 x i64], [2 x i64] } @collapse_shape_dynamic_with_non_identity_layout(ptr %arg, ptr %arg1, i64 %arg2, i64 %arg3, i64 %arg4, i64 %arg5, i64 %arg6, i64 %arg7, i64 %arg8) {
   bb:
  -  %test.not = icmp eq i64 %arg5, 1
  -  br i1 %test.not, label %bb34, label %bb36
  -
  -bb34:                                             ; preds = %bb
  -  br label %bb36
  -
  -bb36:                                             ; preds = %bb34, %bb
  -  %stride1 = phi i64 [ %arg7, %bb34 ], [ %arg8, %bb ]
     %desc = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } undef, ptr %arg, 0
     %desc0 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc, ptr %arg1, 1
     %desc1 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc0, i64 %arg2, 2
     %desc2 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc1, i64 4, 3, 0
     %size1 = mul i64 %arg4, %arg5
     %desc3 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc2, i64 %size1, 3, 1
     %desc4 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc3, i64 %arg6, 4, 0
  -  %desc5 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc4, i64 %stride1, 4, 1
  +  %desc5 = insertvalue { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc4, i64 1, 4, 1
     ret { ptr, ptr, i64, [2 x i64], [2 x i64] } %desc5
   }
   
   !llvm.module.flags = !{!0}
   
   !0 = !{i32 2, !"Debug Info Version", i32 3}

The new code code ditches the check for the dimensions of size one.
That's interesting because the old code was trying to find a "better" stride for the collapsed dimensions, but I believe this is not generally correct and if I understand the "specifications" of collapse shape, this shouldn't be required at all:

  Collapsing non-contiguous dimensions is undefined behavior.

Put differently if we collapse dimensions that are not contiguous, unless I miss something, that means that our stride would have to go through gaps within the same dimension, which we don't support right now. Hence, I don't think the old code was doing the right thing here, but it's late, it's Friday and I may not be thinking clear anymore :).

That code with the check for `stride == 1` comes from https://reviews.llvm.org/D124001.

@cathyzhyi, @springerm, what do you think?
CC'ing: @ftynse and @nicolasvasilache to have a better "letter of the law" semantic of the `collapse_shape` :).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136483