Page MenuHomePhabricator

Enable ReassociatingReshapeOpConversion with "non-identity" layouts.
ClosedPublic

Authored by Benoit on Nov 30 2021, 9:49 AM.

Details

Summary

Enable ReassociatingReshapeOpConversion with "non-identity" layouts.

This removes an early-return in this function, which seems unnecessary and is
preventing some memref.collapse_shape from converting to LLVM (see included lit test).

It seems unnecessary because the return message says "only empty layout map is supported"
but there actually is code in this function to deal with non-empty layout maps. Maybe
it refers to an earlier state of implementation and is just out of date?

Though, there is another concern about this early return: the condition that it actually
checks, {src,dst}MemrefType.getLayout().isIdentity(), is not quite the same as what the
return message says, "only empty layout map is supported". Stepping through this
getLayout().isIdentity() code in GDB, I found that it evaluates to .getAffineMap().isIdentity()
which does (AffineMap.cpp:271):

if (getNumDims() != getNumResults())
  return false;

This seems that it would always return false for memrefs of rank greater than 1 ?

Diff Detail

Event Timeline

Benoit created this revision.Nov 30 2021, 9:49 AM
Benoit requested review of this revision.Nov 30 2021, 9:49 AM
Benoit edited the summary of this revision. (Show Details)Nov 30 2021, 9:51 AM
Benoit added a reviewer: mravishankar.
Benoit updated this revision to Diff 390807.Nov 30 2021, 1:19 PM

Rename the test function - was not match what it actually did

This is due to the weird trifecta between:

  1. memref without an affine_layout map being the convention to represent the canonical contiguous buffer of size equal to "the product of sizes"
  2. memref with an identity affine_map layout being by convention equivalent to no affine_layout
  3. memref with a strided layout that are currently represented by an affine_map.

This also intersects with the fact that a memref layout is provisioned to allow an arbitrarily complex composition of affine maps that I won't even get into.

I forget between the equivalence between 1. and 2. predates the strided memref or not (I think so but I am not 100% sure).

Things will get simpler once strided memref get back to using a different layout attribute offset:x, strides:[x,x,x] as originally intended.
However there are still many skeletons left because in the absence of a richer type/attribute system, it is unknown how the x in the strides relate to the ? in the sizes.
Properly encoding that relationship is key to being airtight in representing contiguous / non-contiguous layouts and reshapes with dynamic shapes.

Based on this info, please feel free to update docs so that it is less surprising to your future self and to others who will approach from the same angle as you did.

Thanks much!

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1171

See my longer comment but please only relax this in the fully static case.

in the absence of a richer type/attribute system, it is unknown how the `x` in the strides relate to the `?` in the sizes.
nicolasvasilache requested changes to this revision.Dec 3 2021, 7:48 AM
This revision now requires changes to proceed.Dec 3 2021, 7:48 AM
Benoit updated this revision to Diff 393903.Dec 13 2021, 8:06 AM

restrict to static shape case

Thanks for the review and explanation. I have updated the code to apply your inline comment i.e. restrict the change to the static shape case.
I haven't updated docs as you suggested because I'm out of my depth here, sorry.

This is due to the weird trifecta between:

  1. memref without an affine_layout map being the convention to represent the canonical contiguous buffer of size equal to "the product of sizes"
  2. memref with an identity affine_map layout being by convention equivalent to no affine_layout
  3. memref with a strided layout that are currently represented by an affine_map.

This also intersects with the fact that a memref layout is provisioned to allow an arbitrarily complex composition of affine maps that I won't even get into.

I forget between the equivalence between 1. and 2. predates the strided memref or not (I think so but I am not 100% sure).

Things will get simpler once strided memref get back to using a different layout attribute offset:x, strides:[x,x,x] as originally intended.
However there are still many skeletons left because in the absence of a richer type/attribute system, it is unknown how the x in the strides relate to the ? in the sizes.
Properly encoding that relationship is key to being airtight in representing contiguous / non-contiguous layouts and reshapes with dynamic shapes.

Based on this info, please feel free to update docs so that it is less surprising to your future self and to others who will approach from the same angle as you did.

Thanks much!

Cant see how this is related to the change here, but I did run into this case today. The below operation currently fails roundtripping

%1 = memref.subview %0 [] [] [] : memref<?x?xf32> to memref<?x?xf32>

since the result subview expects a strided a layout when it shouldnt. I think it is because the subview creation method does not actually adhere to 1 and 2. I was planning to fix that.

This revision is now accepted and ready to land.Dec 14 2021, 10:46 AM

I don't have permissions to submit this myself.

Benoit updated this revision to Diff 399702.Thu, Jan 13, 9:32 AM

rebased and confirmed it is still needed to use vector transfer flattening.

This revision was landed with ongoing or failed builds.Thu, Jan 13, 9:47 AM
This revision was automatically updated to reflect the committed changes.