This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVMIR] Add support for translating Switch instruction
ClosedPublic

Authored by myhsu on Apr 28 2022, 11:32 AM.

Details

Summary

Add support for translating llvm::SwitchInst.

Diff Detail

Event Timeline

myhsu created this revision.Apr 28 2022, 11:32 AM
myhsu requested review of this revision.Apr 28 2022, 11:32 AM
Mogball requested changes to this revision.Apr 28 2022, 11:51 AM
Mogball added a subscriber: Mogball.
Mogball added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
866

add braces

872

spell out the auto

873

add braces

884

llvm::enumerate(swInst->cases()) or llvm::zip?

885

spell out the auto

886

add braces

This revision now requires changes to proceed.Apr 28 2022, 11:51 AM
myhsu updated this revision to Diff 425904.Apr 28 2022, 2:16 PM
myhsu marked 6 inline comments as done.

Addressed coding style issues.

Mogball accepted this revision.Apr 28 2022, 2:48 PM

Thanks for all the fixes/additions to the LLVM importer

This revision is now accepted and ready to land.Apr 28 2022, 2:48 PM
ftynse requested changes to this revision.Apr 29 2022, 1:37 AM

Please add tests for user-visible error messages.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
867–868

The (unwritten) MLIR convention is to have a test for every user-visible error message. Here, I suspect we will see two error messages in a row because whatever failed in processValue should have reported the error as well. Callers usually just propagate the error state.

mlir/test/Target/LLVMIR/Import/switch.ll
6 ↗(On Diff #425904)

Nit: can't this just live with the rest of ops in import.ll?

This revision now requires changes to proceed.Apr 29 2022, 1:37 AM
myhsu updated this revision to Diff 426530.May 2 2022, 3:30 PM
myhsu marked 2 inline comments as done.
  • Integrate test cases into test/Target/LLVMIR/Import/basic.ll
  • Add a new test case to test llvm.switch with arguments
  • Remove error message printing while translating predicate values and block arguments to avoid showing (similar) error messages twice.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
867–868

Here, I suspect we will see two error messages in a row because whatever failed in processValue should have reported the error as well. Callers usually just propagate the error state.

I feel like a better solution will be showing "failed to process the condition value of...", in addition to the error message from processValue, as a note (similar to what clang does) so that user will have a quicker grasp on the problem. But that's a different story I guess.

I will remove the emitError here, the one at line 821 and 836 to avoid confusion (i.e. two error messages). But I'll still add a test case that contains block arguments

ftynse accepted this revision.May 3 2022, 1:34 AM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
867–868

Yes, it would be better to have the general capability of spreading the construction of a diagnostic across multiple calls, but we don't have it now.

This revision is now accepted and ready to land.May 3 2022, 1:34 AM