Page MenuHomePhabricator

[CSKY 6/n] Add support branch and symbol series instruction
ClosedPublic

Authored by zixuan-wu on Jan 20 2021, 2:46 AM.

Details

Summary

This patch adds basic CSKY branch instructions and symbol address series instructions.
Those two kinds of instruction have relationship between each other, and it involves much work about Fixups.

For now, basic instructions are enabled except for disassembler support. We would support to generate basic codegen asm firstly and delay disassembler work later.

Diff Detail

Event Timeline

zixuan-wu created this revision.Jan 20 2021, 2:46 AM
zixuan-wu requested review of this revision.Jan 20 2021, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 2:46 AM
myhsu added inline comments.Jan 26 2021, 9:55 PM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
185

can you condense these three lines into a single return statement?

193

ditto

379

please remove unused comment. Or add FIXME/TODO with explanations

551

can you combine this declaration with line 572? I didn't see any reference of Res until line 572

+1 to @myhsu's points, plus a few comments, but otherwise, is looking good.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp
52

What's the probability that Kind will be within the range? I'm assuming high, so it would be better if return Infos[Kind] was the first thing that checks.

So maybe:

if (FirstTargetFixupKind <= Kind < FirstLiteralRelocationKind)
  return Infos[Kind]
else if (Kind < FirstTargetFixupKind)
  return MCAsmBackend::getFixupKindInfo(Kind);
else
  return MCAsmBackend::getFixupKindInfo(FK_NONE);
llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCCodeEmitter.h
83

Do constant pools and branches really accept any kind of fixup? If not, then maybe an assert on the kinds that work here just to make sure we don't generate bad code.

zixuan-wu updated this revision to Diff 337375.Apr 14 2021, 1:59 AM
rengolin accepted this revision.Apr 16 2021, 4:42 AM

Sorry, you haven't said anything about the comments, I didn't know you had updated the patch to address them.

Phab isn't great at communication, so when you do address all comments, a simple line saying "addressing all comments" helps us know that we should look at it again.

Anyway, LGTM, thanks!

This revision is now accepted and ready to land.Apr 16 2021, 4:42 AM
zixuan-wu marked 5 inline comments as done.Apr 18 2021, 7:35 PM

Sorry, you haven't said anything about the comments, I didn't know you had updated the patch to address them.

Phab isn't great at communication, so when you do address all comments, a simple line saying "addressing all comments" helps us know that we should look at it again.

Anyway, LGTM, thanks!

OK. Thank you for your review. I will mark Done to track every comment next time as the Phab provides the Done button.

This revision was landed with ongoing or failed builds.Apr 20 2021, 12:38 AM
This revision was automatically updated to reflect the committed changes.