This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Support CodeModel::Large codegen
ClosedPublic

Authored by xen0n on May 14 2023, 4:26 AM.

Details

Summary

This is intended to behave like GCC's -mcmodel=extreme.

Technically the true GCC equivalent would be -mcmodel=large which is
not yet implemented there, and we probably do not want to take the
"Large" name until things settle in GCC side, but:

  • LLVM does not have a CodeModel::Extreme, and it seems too early to have such a variant added just for enabling LoongArch; and
  • CodeModel::Small is already being used for GCC -mcmodel=normal which is already a case of divergent naming.

Regarding the codegen, loads/stores immediately after a PC-relative
large address load (that ends with something like `add.d $addr, $addr,
$tmp`) should get merged with the addition into corresponding ldx/stx
ops, but is currently not done. This is because pseudo-instructions are
expanded after instruction selection, and is best fixed with a separate
change.

Diff Detail

Event Timeline

xen0n created this revision.May 14 2023, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2023, 4:26 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
xen0n requested review of this revision.May 14 2023, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2023, 4:26 AM
xen0n planned changes to this revision.May 14 2023, 11:18 PM

Fails bootstrap, pending debug.

benshi001 added inline comments.May 15 2023, 12:46 AM
llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp
211

I think the suffix s is unnecessary for variables. :)

252

Why name it to TmpPart023 rather than TmpPart3 ?

313

Do we also need to add comment about instruction sequence as the following lines ?

380

Do we have to check if it is LA64? Should there be an error if it is LA32+Large ?

benshi001 added a comment.EditedMay 15 2023, 12:55 AM

If LargeModel is not supported on LA32, it would be better to add some asserts or llvm_unreachable to proper places to gurantee that.

xen0n added a comment.May 15 2023, 2:39 AM

Thanks for the reviews. I've fixed the bootstrap bug (GOT access gotcha) and will send another revision soon, after the bootstrapped LLVM passes tests.

llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp
211

Okay, I'll rename these into MO... instead, given the source argument is IdentifyingMO.

252

The name is intended to convey that this temporary value contains the parts 0, 2, and 3 of the desired value (part 1 is loaded with pcalau12i to give PC-relative semantics). Which do you think is better, names like "TmpParts023" (notice the plural "parts"), or simpler names like just "TmpA" "TmpB" and "TmpC"?

313

The large code model sequence is long, but I'll try describing it with a short sentence anyway. Thanks for the suggestion.

380

Good idea, will do.

benshi001 added inline comments.May 15 2023, 3:02 AM
llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp
252

Just my opinion, it would be better that the instruction name and its desitnation register name can be similar.

xen0n updated this revision to Diff 522114.May 15 2023, 3:58 AM
xen0n marked 4 inline comments as done.
  • Rebased
  • Fixed GOT accesses; LLVM is successfully bootstrapped and passes tests with this patch applied
  • The pcalau12i $scratch, ... is now emitted first (to match existing assembler behavior)
  • Added checks and assertions that ensure LA64 when the large code model is selected
  • Comment additions and value renamings per review
xen0n updated this revision to Diff 522118.May 15 2023, 4:09 AM

fix missing load in TLS IE

xen0n added a comment.May 15 2023, 4:32 AM

Actually I'm not sure if reusing the PseudoLA_*_LARGE definitions is a good idea. They seem to be there simply for the assembler's needs, and it may be better to add a set of isCodeGenOnly = 1 Pseudo's to avoid having to build large amounts of unused 0 nodes like now.

hev added a subscriber: hev.May 25 2023, 12:07 AM
SixWeining accepted this revision.May 29 2023, 1:05 AM

Thanks. LGTM with some nits.

llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp
266

It's better to use complete sentence.

361

Do you mean Parts012 ?

366

Ditto.

This revision is now accepted and ready to land.May 29 2023, 1:05 AM
xen0n updated this revision to Diff 533165.Jun 21 2023, 12:34 AM
xen0n marked 3 inline comments as done.

rebase and address review comments

SixWeining accepted this revision.Jun 21 2023, 1:42 AM

Thanks.

This revision was automatically updated to reflect the committed changes.