Page MenuHomePhabricator

Implement callee/caller type checking for llvm.call
ClosedPublic

Authored by mehdi_amini on Sep 26 2020, 10:14 AM.

Details

Summary

This aligns the behavior with the standard call as well as the LLVM verifier.

Diff Detail

Event Timeline

mehdi_amini created this revision.Sep 26 2020, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2020, 10:14 AM
mehdi_amini requested review of this revision.Sep 26 2020, 10:14 AM
mehdi_amini added inline comments.
mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)

@dcaballe : this is the problematic call right now.

Replace some auto with explicit types

dcaballe accepted this revision.Sep 26 2020, 8:52 PM

Thanks, LGTM! Please, give some time for @ftynse to have a look.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
541

it an -> it is an?

547

remove {}?

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)

Thanks! I should be able to enable this after https://reviews.llvm.org/D87724.

This revision is now accepted and ready to land.Sep 26 2020, 8:52 PM
dcaballe added inline comments.Sep 29 2020, 12:19 PM
mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)

D87724 landed already. If you uncomment the scf/standard dialect main function above and remove the llvm one, this seems to work now. Could you please rebase your patch and give it a try?

mehdi_amini added inline comments.Sep 29 2020, 1:19 PM
mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)

Yes it works, but that seems also like orthogonal to the current patch, why didn't you do it as part of D87724?

mehdi_amini added inline comments.Sep 29 2020, 1:22 PM
mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)
mehdi_amini edited the summary of this revision. (Show Details)

Rebase, test is passing so removing XFAIL

Could we have tests for user-visible error messages?

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
542

Using textual attribute names makes it feel like JSON. We can just have op.callee() or op.calleeAttr() if the attribute object is necessary.

Also, this seems to accept nested symbol references without verifying them.

dcaballe added inline comments.Oct 1 2020, 4:09 PM
mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
143 ↗(On Diff #294507)

Yes it works, but that seems also like orthogonal to the current patch, why didn't you do it as part of D87724?

Sorry, not intentional. I thought it could be some interaction with your patch. Thanks for fixing it.

Add tests and add verifier for indirect calls

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
542

Copied it out the textual part from the Standard dialect verifier ;)

(the nested symbol should be covered by the native verifier from the ODS definition specifying a FlatSymbolRefAttr)

ftynse accepted this revision.Oct 2 2020, 12:24 AM

Thanks!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
542

Ah, we need to port the Standard dialect to (modern) ODS :)

This revision was automatically updated to reflect the committed changes.