-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SOL] Remove lddw
instruction
#74
[SOL] Remove lddw
instruction
#74
Conversation
(outs GPR:$dst), | ||
(ins GPR:$src2, u64imm:$imm), | ||
"hor64 $dst, $imm", | ||
[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this isn't incorrect per-se, we do not need two different instructions defined for each case (different selection patterns are enough). Optional change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't see how to do it with patterns. The two instructions have different operand types, and I get and error if I use only HOR
.
llvm/lib/Target/SBF/SBFInstrInfo.td
Outdated
// These instructions copy the value 0x1122334455667788 to a register. | ||
def : Pat<(i64 imm:$imm), | ||
(HOR (MOV_32_64_imm (i32 (Lower64 $imm))), | ||
(i32 (Upper64 $imm)))>, Requires<[SBFv2]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that since we now generate the pair rather than lddw
, there are opportunities for minor optimizations here. For example, if the upper 32-bits is clear, we only need to emit one instruction (and similar patterns).
It isn't important for this patch, but a follow-up patch might add those tweaks and corresponding unit tests. Given the absurdly long instruction word size of BPF/SBF (and the code size restrictions of Solana contracts/programs), eliminating redundant instructions like this is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the few minor NFC changes needed before commit, LGTM.
4ca616d
into
anza-xyz:solana-rustc/16.0-2023-06-05
* Remove lddw instruction * Add suggestions
* Remove lddw instruction * Add suggestions
* Remove lddw instruction * Add suggestions
* Remove lddw instruction * Add suggestions
This PR represents the third task in solana-labs/solana#34250. It removes the
lddw
instructions and configures the SBF backend to generatemov32
andhor64
for loading 64-bit immediates.It is the corresponding change in LLVM for solana-labs/rbpf#486 and solana-labs/rbpf#496.