[mlir][NFC] Use `BaseMemRefType::isValidElementType` in Parser

[vinograd47]

Summary

Instead of hard-coded check.

[ftynse]

Move BaseMemRefType::isValidElementType definition to CPP file.
It will allow to extend it in future or in custom forks without
affecting header file.

I am not convinced by this motivation. If a fork wants to diverge from how built-in types are handled, it can also do the necessary code motion locally. There is also a proper way of adding support for custom elements in memref, which relies on the data layout that is in process of being added. This has been discussed numerous times and is mostly a matter of proper implementation (not just removing the check)...

That being said, we should change the parser.

[vinograd47]

@ftynse I saw the proposal about data layout and custom elements. How it assume to perform the validity check? I thought it will extend the existing isValidElementType method with more advanced checks. IMHO, in that case it will be simpler to do this in CPP file, otherwise it might bring extra dependencies to header file or make the method to complex to be inline. That's why I've moved its implementation to CPP.

[rriddle]

In D97612#2593575, @ftynse wrote:

Move BaseMemRefType::isValidElementType definition to CPP file.
It will allow to extend it in future or in custom forks without
affecting header file.

I am not convinced by this motivation. If a fork wants to diverge from how built-in types are handled, it can also do the necessary code motion locally. There is also a proper way of adding support for custom elements in memref, which relies on the data layout that is in process of being added. This has been discussed numerous times and is mostly a matter of proper implementation (not just removing the check)...

That being said, we should change the parser.

+1 to what Alex says here.

mlir/lib/IR/BuiltinTypes.cpp

475 ↗

(On Diff #326884)

I don't really see a valid upstream reason why this needs to move out-of-line.

[ftynse]

In D97612#2594598, @vinograd47 wrote:

@ftynse I saw the proposal about data layout and custom elements. How it assume to perform the validity check? I thought it will extend the existing isValidElementType method with more advanced checks. IMHO, in that case it will be simpler to do this in CPP file, otherwise it might bring extra dependencies to header file or make the method to complex to be inline. That's why I've moved its implementation to CPP.

The simplest option will be just having type.isa<DataLayoutTypeInterface>() assuming built-in types implement the interface, or a dedicated MemRefElementInterface. I intentionally did not propose a specific mechanism because it deserves a proper discussion, especially wrt layering. When we reach a consensus on that, we may have a valid upstream reason to uninline the method.

[vinograd47]

Revert move from HPP to CPP.

[vinograd47]

@ftynse @rriddle I've reverted the method move to CPP file. Are you OK with its usage in Parser instead of duplicating check code?

[rriddle]

LGTM

[vinograd47]

Rebased

[vinograd47]

Rebased

mlir/lib/Parser/TypeParser.cpp

Type Parser::parseMemRefType() {
 
   // Parse the element type.
   auto typeLoc = getToken().getLoc();
   auto elementType = parseType();
   if (!elementType)
     return nullptr;
 
   // Check that memref is formed from allowed types.
-  if (!elementType.isIntOrIndexOrFloat() &&
-      !elementType.isa<VectorType, ComplexType>())
+  if (!BaseMemRefType::isValidElementType(elementType))
     return emitError(typeLoc, "invalid memref element type"), nullptr;
 
   // Parse semi-affine-map-composition.
   SmallVector<AffineMap, 2> affineMapComposition;
   Optional<unsigned> memorySpace;
   unsigned numDims = dimensions.size();
 
   auto parseElt = [&]() -> ParseResult {