This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Fixup value adjustment in applyFixup
ClosedPublic

Authored by wangleiat on Aug 28 2022, 8:03 PM.

Details

Summary

A complete implementation of applyFixup for D132323.

Makes LoongArchAsmBackend::shouldForceRelocation to determine
if the relocation types must be forced.

This patch also adds range and alignment checks for b* instructions'
operands, at which point the offset to a label is known.

Depends on D132633

Diff Detail

Event Timeline

wangleiat created this revision.Aug 28 2022, 8:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 8:03 PM
wangleiat requested review of this revision.Aug 28 2022, 8:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 8:03 PM
xen0n added inline comments.Aug 30 2022, 7:47 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
139–143

This is placed here due to review comment on D132323, does the situation change enough to make this change appropriate?

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
251

Do we want to also handle BL here? Given it's the only other insn of the format.

wangleiat added inline comments.Aug 30 2022, 8:23 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
139–143

Personal preference, just put the processing of Value together (Increases the readability of the code). Maybe it was put in the front at the time because of performance? If you think this is not good, I can modify it back.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
251

Do we want to also handle BL here? Given it's the only other insn of the format.

It is not needed. This is because BL are handled specifically in both the LowerCall phase and ASMParser.

xen0n added inline comments.Aug 30 2022, 8:43 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
139–143

Granted it's just a teensy bit of performance, but it's still @SixWeining's input after all, and after the change it's back to the same as RISCV, and effectively code churn for no objective reason. So I'd personally prefer the 2 lines be moved back. Thanks!

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
251

Okay, thanks for the explanation. I have no problem with that.

wangleiat updated this revision to Diff 456850.Aug 30 2022, 9:08 PM

Address @xen0n's comments.

xen0n accepted this revision.Aug 30 2022, 9:10 PM

Looks reasonable now. Thanks!

This revision is now accepted and ready to land.Aug 30 2022, 9:10 PM
MaskRay requested changes to this revision.Aug 30 2022, 10:22 PM

Can some use FirstLiteralRelocationKind + ... instead?

This revision now requires changes to proceed.Aug 30 2022, 10:22 PM
wangleiat added a comment.EditedAug 31 2022, 12:23 AM

Can some use FirstLiteralRelocationKind + ... instead?

Great! Will omit a lot of code, I will do that.

To avoid a lot of code removal, I'm going to make changes in the commits it depends on D132633

MaskRay requested changes to this revision.Sep 1 2022, 8:46 PM
MaskRay added inline comments.
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
93

% 4. The compiler can optimize it

100

% 4

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
267

break;

llvm/test/MC/LoongArch/Relocations/fixups-diagnostics.s
2

|& => 2>&1 |

Some people's default shell (e.g. some macOS) don't support |&

4

#@LINE

@LINE is deprecated

This revision now requires changes to proceed.Sep 1 2022, 8:46 PM

Thanks for all the comments, I will fix them. :)

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
93

% 4. The compiler can optimize it

100

% 4

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
267

break;

llvm/test/MC/LoongArch/Relocations/fixups-diagnostics.s
2

|& => 2>&1 |

Some people's default shell (e.g. some macOS) don't support |&

4

#@LINE

@LINE is deprecated

wangleiat updated this revision to Diff 457511.Sep 1 2022, 11:39 PM

Address @MaskRay's comments.

wangleiat updated this revision to Diff 457529.Sep 2 2022, 1:27 AM

Fix [[@LINE]] what was missed.

MaskRay accepted this revision.Sep 13 2022, 10:19 PM
This revision is now accepted and ready to land.Sep 13 2022, 10:19 PM
This revision was automatically updated to reflect the committed changes.