[mlir] StdToLLVM: Add error when the sourceMemRef of a subview is not a llvm type.

[poechsel]

Summary

A memref_cast casting to a memref with a non identity map can't be lowered to llvm. Take the following test case:

#map1 = affine_map<(d0, d1)[s0, s1, s2] -> (d0 * s1 + s0 + d1 * s2)>

func @invalid_memref_cast(%arg0: memref<?x?xf64>) {
  %c1 = constant 1 : index
  %c0 = constant 0 : index
  %5 = memref_cast %arg0 : memref<?x?xf64> to memref<?x?xf64, #map1>
  %25 = std.subview %5[%c0, %c0][%c1, %c1][] : memref<?x?xf64, #map1> to memref<?x?xf64, #map1>
  return
}

When lowering the subview mlir was assuming %5 to have an llvm type (which is not the case as mlir failed to lower the memref_cast).

[ftynse]

Nit: please explain in the commit message that this was segfaulting before your change, and why it was. Or, as https://mlir.llvm.org/getting_started/DeveloperGuide/ states it, the commit message should explain _why_ you are making the change.

[mehdi_amini]

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

389

Something does not line up here: you are using dyn_cast which is supposed to be expecting a possible nullptr as a result, but then you add adding an assert, which implies this is an impossible case...

If you are using the assert just to get a “nicer” crash, then the pattern should be asserting on “isa<>” and using cast<>

[ftynse]

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

389

It is indeed for a more explicative assert message.

What you say sort of contradicts http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates which says "you should not use an isa<> test followed by a cast<>".

[mehdi_amini]

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

389

You're right: but that is because the correct way is to use cast!

I'm only mentioning the the isa<> is only because you want a nicer error message with the assert, otherwise cast<> is idiom to follow.

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

LLVMOpLowering::LLVMOpLowering(StringRef rootOpName, MLIRContext *context,
                                PatternBenefit benefit)
     : ConversionPattern(rootOpName, benefit, context), lowering(lowering_) {}
 
 /*============================================================================*/
 /* StructBuilder implementation                                               */
 /*============================================================================*/
 StructBuilder::StructBuilder(Value v) : value(v) {
   assert(value != nullptr && "value cannot be null");
-  structType = value.getType().cast<LLVM::LLVMType>();
+  structType = value.getType().dyn_cast<LLVM::LLVMType>();
+  assert(structType && "expected llvm type");
 }
 
 Value StructBuilder::extractPtr(OpBuilder &builder, Location loc,
                                 unsigned pos) {
   Type type = structType.cast<LLVM::LLVMType>().getStructElementType(pos);
   return builder.create<LLVM::ExtractValueOp>(loc, type, value,
                                               builder.getI64ArrayAttr(pos));
 }
if (failed(successStrides))
       return matchFailure();
 
     // Fail to convert if neither a dynamic nor static offset is available.
     if (dynamicOffsets.empty() &&
         offset == MemRefType::getDynamicStrideOrOffset())
       return matchFailure();
 
     // Create the descriptor.
+    if (!operands.front().getType().isa<LLVM::LLVMType>())
+      return matchFailure();
     MemRefDescriptor sourceMemRef(operands.front());
     auto targetMemRef = MemRefDescriptor::undef(rewriter, loc, targetDescTy);
 
     // Copy the buffer pointer from the old descriptor to the new one.
     Value extracted = sourceMemRef.allocatedPtr(rewriter, loc);
     Value bitcastPtr = rewriter.create<LLVM::BitcastOp>(
         loc, targetElementTy.getPointerTo(), extracted);
     targetMemRef.setAllocatedPtr(rewriter, loc, bitcastPtr);

mlir/test/Conversion/StandardToLLVM/invalid.mlir

+// RUN: mlir-opt %s -verify-diagnostics -split-input-file
+
+#map1 = affine_map<(d0, d1)[s0, s1, s2] -> (d0 * s1 + s0 + d1 * s2)>
+
+func @invalid_memref_cast(%arg0: memref<?x?xf64>) {
+  %c1 = constant 1 : index
+  %c0 = constant 0 : index
+  // expected-error@+1: 'std.memref_cast' op operand #0 must be unranked.memref of any type values or memref of any type values,
+  %5 = memref_cast %arg0 : memref<?x?xf64> to memref<?x?xf64, #map1>
+  %25 = std.subview %5[%c0, %c0][%c1, %c1][] : memref<?x?xf64, #map1> to memref<?x?xf64, #map1>
+  return
+}
+