This is an archive of the discontinued LLVM Phabricator instance.

[M68k][GloballSel] RegBankSelect implementation
ClosedPublic

Authored by sushmaunnibhavi on Aug 5 2021, 2:02 AM.

Details

Summary

Implementation of RegBankSelect for the M68k backend.

Diff Detail

Event Timeline

sushmaunnibhavi created this revision.Aug 5 2021, 2:02 AM
sushmaunnibhavi requested review of this revision.Aug 5 2021, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2021, 2:02 AM
gandhi21299 added inline comments.Aug 5 2021, 9:00 AM
llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp
80

I don't see MRI used anywhere

llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.h
46

please add a newline here

llvm/test/CodeGen/M68k/GlobalISel/reg_bank_test.ll
5

please add more CHECKS for MIR

Updated patch according to @gandhi21299 comments.

sushmaunnibhavi marked 3 inline comments as done.Aug 6 2021, 12:04 AM

@myhsu , @gandhi21299 can you please review the patch?

myhsu added inline comments.Aug 8 2021, 2:44 PM
llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp
96

Why line 86, 92, and 96 are the same? Can we combine them?

llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.h
44

why change this?

llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp
96

Yes it can be combined.

Updated patch according to @myhsu comments.

sushmaunnibhavi marked an inline comment as done.Aug 9 2021, 12:21 AM
myhsu added inline comments.Aug 9 2021, 2:14 PM
llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp
49

Is it better to say "3 operands" instead of "3 ops" here? Because at first I thought it means 3 operators.

Updated patch according to @myhsu comments.

sushmaunnibhavi marked an inline comment as done.Aug 9 2021, 7:54 PM
myhsu accepted this revision.Aug 9 2021, 11:30 PM

LGTM Cheers!
Please fix the unused variable though

llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp
79

This variable is not used anywhere

This revision is now accepted and ready to land.Aug 9 2021, 11:30 PM
myhsu added a comment.Aug 9 2021, 11:32 PM

And also please provide a summary of this patch

sushmaunnibhavi edited the summary of this revision. (Show Details)

Removed unused variables.

sushmaunnibhavi marked an inline comment as done.Aug 10 2021, 12:45 AM
This revision was landed with ongoing or failed builds.Aug 10 2021, 3:28 PM
This revision was automatically updated to reflect the committed changes.