This is an archive of the discontinued LLVM Phabricator instance.

[M68k] Add MC support for link/unlk
ClosedPublic

Authored by 0x59616e on May 11 2022, 8:10 PM.

Diff Detail

Event Timeline

0x59616e created this revision.May 11 2022, 8:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:11 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
0x59616e requested review of this revision.May 11 2022, 8:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:11 PM
myhsu added a comment.May 26 2022, 9:18 AM

Thank you for working on these instructions. I will recommend to add codegen support in this patch as well.

llvm/lib/Target/M68k/M68kInstrData.td
446

According to https://m680x0.github.io/ref/integer-instructions.html#pfd7 , LINK pushes register onto the stack, so I think the address register should be the input.

Thank you for working on these instructions. I will recommend to add codegen support in this patch as well.

No problem ;)

0x59616e added inline comments.May 26 2022, 11:14 PM
llvm/lib/Target/M68k/M68kInstrData.td
446

It does read and write to the register. So I guess it should be both input and output ?

0x59616e updated this revision to Diff 445459.Jul 18 2022, 4:55 AM

Adopt link/unlink in stack prologue/epilogue

Something worth noting about:

  • I guess the ".cfi_*" stuff may be wrong. Not sure about it.
  • The assembly in link-unlink.ll, other than link/unlink itself, seems to be wrong. Bugs may exist somewhere.
myhsu accepted this revision.Jul 31 2022, 12:25 PM

LGTM
Thank you! I only have some minor comments.

Something worth noting about:

  • I guess the ".cfi_*" stuff may be wrong. Not sure about it.
  • The assembly in link-unlink.ll, other than link/unlink itself, seems to be wrong. Bugs may exist somewhere.

Yeah we can fix those later.

llvm/test/CodeGen/M68k/link-unlnk.ll
3

Can we have checks for cases in the absence of frame pointer, too? One way to do this is using the command line flag -frame-pointer=none/all to toggle frame pointer instead of using the function attribute (i.e. #0).

llvm/test/MC/M68k/Data/Classes/MxLink.s
2

nit: remove -assemble

This revision is now accepted and ready to land.Jul 31 2022, 12:25 PM
0x59616e updated this revision to Diff 450681.Aug 7 2022, 7:01 PM
0x59616e marked 3 inline comments as done.

update diff with the following modification:

  • test file link-unlink.ll has been updated to cover the cases with or without frame-pointer
  • test file MxLink.s has been updated as per the suggestion by min.

LGTM
Thank you! I only have some minor comments.

Something worth noting about:

  • I guess the ".cfi_*" stuff may be wrong. Not sure about it.
  • The assembly in link-unlink.ll, other than link/unlink itself, seems to be wrong. Bugs may exist somewhere.

Yeah we can fix those later.

I'll open a issue citing this problem.

This revision was landed with ongoing or failed builds.Aug 7 2022, 8:01 PM
This revision was automatically updated to reflect the committed changes.