This is an archive of the discontinued LLVM Phabricator instance.

[llvm][LoongArch] Improve the "out of range" error information reported by `adjustFixupValue`
ClosedPublic

Authored by XiaodongLoong on Oct 25 2022, 11:41 PM.

Details

Summary

There are three reduplicate error messages for different conditions. I
add meaningful information to make them more informative.

Diff Detail

Event Timeline

XiaodongLoong created this revision.Oct 25 2022, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 11:41 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
XiaodongLoong requested review of this revision.Oct 25 2022, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 11:41 PM
SixWeining added inline comments.Oct 25 2022, 11:46 PM
llvm/test/MC/LoongArch/Relocations/fixups-diagnostics.s
1

Seems you are using an old code base.

SixWeining added inline comments.Oct 26 2022, 12:11 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
98

Should be "bits" or something else?

XiaodongLoong added inline comments.Oct 26 2022, 12:12 AM
llvm/test/MC/LoongArch/Relocations/fixups-diagnostics.s
1

Yes, I rebased code.

MaskRay added inline comments.Oct 26 2022, 11:41 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
98

You can define a method to return a string fixup value out of range [-xxx, xxx]

, NFC

There is a functional change, so not NFC.

Update patch for review comments.

XiaodongLoong retitled this revision from [llvm][LoongArch] Improve the "out of range" error information reported by `adjustFixupValue`, NFC to [llvm][LoongArch] Improve the "out of range" error information reported by `adjustFixupValue`.Oct 27 2022, 6:50 PM
XiaodongLoong marked 2 inline comments as done.

I defined a method named "reportOutOfRangeError" to report "out of range" error. Thanks!

MaskRay accepted this revision.Oct 27 2022, 7:02 PM

LGTM.

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

Remove the two variables. Use llvm::minIntN(N) and llvm::maxIntN(N) in `Twine.

85

Remove .str()

This revision is now accepted and ready to land.Oct 27 2022, 7:02 PM

Use llvm::minIntN(N) and llvm::maxIntN(N) and remove .str()

XiaodongLoong marked 2 inline comments as done.Oct 27 2022, 7:28 PM

Use llvm::minIntN(N) and llvm::maxIntN(N) and remove .str(). Done.

MaskRay added inline comments.Oct 27 2022, 7:37 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
82

The function just uses the Location, so a SMLoc parameter is more appropriate.
MCContext is usually added as the first parameter.

Change parameters type and order

XiaodongLoong marked an inline comment as done.Oct 28 2022, 12:36 AM
SixWeining accepted this revision.Oct 28 2022, 6:09 PM