Page MenuHomePhabricator

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

Authored by poechsel on Wed, Feb 12, 12:46 AM.

Details

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).

Diff Detail

Event Timeline

poechsel created this revision.Wed, Feb 12, 12:46 AM
ftynse accepted this revision.Wed, Feb 12, 12:58 AM

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.

This revision is now accepted and ready to land.Wed, Feb 12, 12:58 AM
poechsel edited the summary of this revision. (Show Details)Wed, Feb 12, 1:09 AM
poechsel edited the summary of this revision. (Show Details)
poechsel edited the summary of this revision. (Show Details)
poechsel edited the summary of this revision. (Show Details)Wed, Feb 12, 1:13 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Wed, Feb 12, 11:17 PM
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 added inline comments.Thu, Feb 13, 12:11 AM
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 added inline comments.Thu, Feb 13, 8:01 PM
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.