This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Add EncoderMethods for transformed immediate operands
ClosedPublic

Authored by SixWeining on Feb 25 2022, 1:24 AM.

Details

Summary

This is a split patch of D120476 and thanks to myhsu.

'Transformed' means the encoding of an immediate is not the same as
its binary representation. For example, the bl instruction
requires a signed 28-bits integer as its operand and the low 2 bits
must be 0. So only the upper 26 bits are needed to get encoded into
the instruction.

Based on the above reason this kind of immediate needs a customed
EncoderMethod to get the real value getting encoded into the
instruction.

Currently these immediate includes:

uimm2_plus1
simm14_lsl2
simm16_lsl2
simm21_lsl2
simm26_lsl2

This patch adds those EncoderMethods and revises related .mir test
in previous patch.

Diff Detail

Event Timeline

SixWeining created this revision.Feb 25 2022, 1:24 AM
SixWeining requested review of this revision.Feb 25 2022, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 1:24 AM
myhsu added inline comments.Feb 25 2022, 3:17 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
55

are these two functions dedicated to the unusual immediate you mentioned in another patch? If so, can you add some description comments here?

Add description comments to the 2 new functions to address @myhsu's comments.

SixWeining added inline comments.Feb 26 2022, 1:15 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
55

Yes, they are dedicated to those unusual immediates. I just add some descriptions which I think may improve code readability. Many thanks!

SixWeining edited the summary of this revision. (Show Details)Feb 26 2022, 1:18 AM
xen0n added a comment.Feb 27 2022, 7:27 AM

Maybe something like "transformed operands" is better than "unusual", for the changeset title? The code looks good though, only one minor nit.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
64–65
/// The value returned is the value of the immediate shifted right
/// arithmetically by 2.

Per @xen0n's comments, change the title from 'unusual' to 'transformed'. Thanks!

Fix the nit. Sorry I missed it in last revision.

SixWeining retitled this revision from [LoongArch] Add EncoderMethod to unusual' immediate operand to [LoongArch] Add EncoderMethod to transformed immediate operand.Feb 27 2022, 5:32 PM
SixWeining edited the summary of this revision. (Show Details)
xen0n accepted this revision.Feb 27 2022, 7:28 PM

The code changes seem good to me. Thanks for fixing the nit!

The title is still somewhat Chinglish-sounding though (wrong plurals), "[LoongArch] Add EncoderMethods for transformed immediate operands" is better.

(Still, approval from someone else would be nice before merging, as usual.)

This revision is now accepted and ready to land.Feb 27 2022, 7:28 PM
SixWeining retitled this revision from [LoongArch] Add EncoderMethod to transformed immediate operand to [LoongArch] Add EncoderMethods for transformed immediate operands.Feb 27 2022, 7:59 PM

The code changes seem good to me. Thanks for fixing the nit!

The title is still somewhat Chinglish-sounding though (wrong plurals), "[LoongArch] Add EncoderMethods for transformed immediate operands" is better.

(Still, approval from someone else would be nice before merging, as usual.)

Yes, that looks good. Thanks.

MaskRay added inline comments.Feb 27 2022, 8:42 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
93

delete unneeded blank line

96

Inline used-once variable

100

llvm_unreachable does not need a following return

Optional: For regular diagnostics, see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages for the recommended style. I know that some llvm_unreachable used capitalized messages.

116

llvm_unreachable does not need a following return

Address @MaskRay's comments. Thanks!

Delete unneeded blank line and returns.

SixWeining marked 4 inline comments as done.Feb 27 2022, 11:17 PM

Thanks! Just updated the revision.

MaskRay accepted this revision.Feb 27 2022, 11:30 PM

With the nit, this looks good to me too, thanks!

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

Not sure what's better here, assert(MO.isImm()) or llvm_unreachable.

On the function above, you return 0 even after llvm_unreachable, but here you don't.

Is that because of compiler warnings? I believe this shouldn't be necessary, but I could be wrong.

SixWeining added inline comments.Feb 28 2022, 6:29 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
96

Seems assert is better because this function is dedicated for the uimm2_plus1 immideate operand. It's impossible to be other operand.

As @MaskRay said, llvm_unreachable does not need a following return. In llvm/include/llvm/Support/ErrorHandling.h, I see:

/// This function calls abort(), and prints the optional message to stderr.
/// Use the llvm_unreachable macro (that adds location info), instead of
/// calling this function directly.
[[noreturn]] void
llvm_unreachable_internal(const char *msg = nullptr, const char *file = nullptr,
                          unsigned line = 0); 
}

/// Marks that the current location is not supposed to be reachable.
/// In !NDEBUG builds, prints the message and location info to stderr.
/// In NDEBUG builds, becomes an optimizer hint that the current location
/// is not supposed to be reachable.  On compilers that don't support
/// such hints, prints a reduced message instead and aborts the program.
///
/// Use this instead of assert(0).  It conveys intent more clearly and
/// allows compilers to omit some unnecessary code.
#ifndef NDEBUG
#define llvm_unreachable(msg) \
  ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
#elif defined(LLVM_BUILTIN_UNREACHABLE)
#define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
#else
#define llvm_unreachable(msg) ::llvm::llvm_unreachable_internal()
#endif

#endif

Here llvm_unreachable_internal() is marked with [[noreturn]].

As the above function, since it's not introduced in this patch, I will delete the unnecessary return 0 in a seperate patch.

Use assert instead of if()... to check input.

MaskRay added inline comments.Mar 1 2022, 8:53 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
93

Just use return MO.getImm() - 1;

getImm has an internal assert(isImm() && ...

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 8:53 PM
MaskRay requested changes to this revision.Mar 1 2022, 8:54 PM
MaskRay added inline comments.
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp
104

Delete assert. getImm has an internal assert

This revision now requires changes to proceed.Mar 1 2022, 8:54 PM
SixWeining marked 2 inline comments as done.

Address @MaskRay's comments.

Delete redundant assert.

Thanks. I have updated the revision.

MaskRay accepted this revision.Mar 5 2022, 9:47 AM

LGTM.

This revision is now accepted and ready to land.Mar 5 2022, 9:47 AM
This revision was landed with ongoing or failed builds.Mar 7 2022, 12:50 AM
This revision was automatically updated to reflect the committed changes.