Page MenuHomePhabricator

[mlir] Fix of updating function signature in normalizing memrefs
ClosedPublic

Authored by imaihal on Sep 16 2020, 12:53 AM.

Details

Summary

Normalizing memrefs failed when a caller of symbolic use in a function
can not be casted to CallOp. This patch avoids the failure by checking
the result of the casting. If the caller can not be casted to CallOp,
it is skipped.

Diff Detail

Event Timeline

imaihal created this revision.Sep 16 2020, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 12:53 AM
imaihal requested review of this revision.Sep 16 2020, 12:53 AM

This bug was discussed here https://llvm.discourse.group/t/memrefs-and-maps-for-tiling/1279/21
Following example failed because spv.EntryPoint "GLCompute" @empty can't be casted to CallOp. I created the example to reproduce errors happened in my code which uses EntryPoint op of my own dialect. This example may not be practical, but this reproduced the same error with my code.

#map0 = affine_map<(d0, d1) -> (d0 floordiv 32, d1 floordiv 64, d0 mod 32, d1 mod 64)>

module {
  func @empty() {
    %0 = alloc() : memref<10x10xf32, #map0>
    return
  }
  spv.EntryPoint "GLCompute" @empty
}

@bondhugula @avarmapml Could you review this patch?

bondhugula requested changes to this revision.EditedSep 16 2020, 2:09 AM

Thanks for raising this. Please add a test case. It's not fully clear one could just ignore all uses that aren't CallOps. Please also add a comment.

This revision now requires changes to proceed.Sep 16 2020, 2:09 AM
bondhugula added inline comments.Sep 16 2020, 2:13 AM
mlir/lib/Transforms/NormalizeMemRefs.cpp
268–272

Please rename variables for better readability.

callOp -> userOp
castCallOp -> callOp

Also, please invert the logic for clarity.

auto callOp = dyn_cast<CallOp>(userOp);
if (!callOp)
  continue;
StringRef callee = castCallOp.getCallee();
imaihal updated this revision to Diff 292408.EditedSep 16 2020, 10:59 PM

Added test and comments, and renamee variables to reflrect comments

imaihal marked an inline comment as done.Sep 16 2020, 11:00 PM

Thank you for raising this.
Solution looks good to me. Please add the test case though.

@bondhugula @avarmapml Thanks for your comments! I added test and comments, and renamed the variables to reflect @bondhugula 's comments.

avarmapml added inline comments.Sep 16 2020, 11:14 PM
mlir/lib/Transforms/NormalizeMemRefs.cpp
273–274

You can rephrase this line as :-
TODO: Handle cases where a non-CallOp symbol use of a function deals with memrefs.

bondhugula accepted this revision.Sep 16 2020, 11:33 PM
bondhugula added inline comments.
mlir/test/Transforms/normalize-memrefs-ops.mlir
59 ↗(On Diff #292408)

-> Test with an arbitrary op that references the symbol
?

Nit: Terminate all comments with period.

mlir/test/lib/Dialect/Test/TestOps.td
631 ↗(On Diff #292408)

Nit: terminate comment with a period.

631 ↗(On Diff #292408)

including entrypoint -> including a reference to a function symbol.

642 ↗(On Diff #292408)

Nit: Drop extra blank line.

This revision is now accepted and ready to land.Sep 16 2020, 11:33 PM
imaihal updated this revision to Diff 292411.Sep 16 2020, 11:34 PM

Rebased and rephrased the comment.

Sorry, I may did wrong operaton... I will fix this.

imaihal updated this revision to Diff 292414.Sep 16 2020, 11:50 PM
imaihal marked an inline comment as done.

Fixed comments and format

imaihal marked 4 inline comments as done.Sep 16 2020, 11:52 PM
bondhugula requested changes to this revision.Sep 17 2020, 2:10 AM
bondhugula added inline comments.
mlir/test/lib/Dialect/Test/TestOps.td
635 ↗(On Diff #292414)

It's still not clear what this means. Did you mean to say it's an arbitrary op with a reference to a function symbol? Why is even an entry point in the context here?

This revision now requires changes to proceed.Sep 17 2020, 2:10 AM
imaihal added inline comments.Sep 17 2020, 7:29 AM
mlir/test/lib/Dialect/Test/TestOps.td
635 ↗(On Diff #292414)

A op for entry point issued the error in my actual use case. So, I created the test op for entry point, but as you said, we should make it more general.

OPEntryPoint -> OPFuncRef
op_entrypoint -> op_funcref
The "test.op_entrypoint" function indicates the main entry point.
-> The "test.op_funcref" function has a reference to a function symbol
How about this?

imaihal updated this revision to Diff 292512.Sep 17 2020, 8:11 AM

Updated comments and rename test func

imaihal marked an inline comment as done.Sep 17 2020, 8:30 AM
bondhugula accepted this revision.Sep 18 2020, 2:59 AM

Thanks!

This revision is now accepted and ready to land.Sep 18 2020, 2:59 AM

@imaihal Thanks for the upgrade. Minor nit still remains, please fix.

mlir/test/lib/Dialect/Test/TestOps.td
631 ↗(On Diff #292512)

@imaihal Please add the period.

imaihal marked an inline comment as done.Sep 18 2020, 3:31 PM
imaihal added inline comments.
mlir/test/lib/Dialect/Test/TestOps.td
631 ↗(On Diff #292512)

@AlexEichenberger This is a two line-comment. Is it ok as it is?

// Test for memrefs normalization of an op with a reference to a function
// symbol.

Thanks!

imaihal marked an inline comment as done.Sep 22 2020, 5:44 PM

@bondhugula @AlexEichenberger @avarmapml Do you have any other comments?

I think I don't have right to commit. So, could you commit this patch if this is ready?

@bondhugula It seems there is no additional comments. Could you commit also this patch?

bondhugula accepted this revision.Sep 25 2020, 10:18 AM