This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][RISCV] fix the extractBits behavior and add R_RISCV_JAL relocation.
ClosedPublic

Authored by fourdim on Jan 22 2022, 8:38 PM.

Details

Summary

This patch supports the R_RISCV_JAL relocation.
Moreover, it will fix the extractBits function's behavior as it extracts Size + 1 bits.
In the test ELF_jal.s:
Before:

Hi: 4294836480
extractBits(Hi, 12, 8): 480

After:

Hi: 4294836480
extractBits(Hi, 12, 8): 224

Diff Detail

Event Timeline

fourdim created this revision.Jan 22 2022, 8:38 PM
fourdim requested review of this revision.Jan 22 2022, 8:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2022, 8:38 PM
StephenFan added inline comments.Jan 23 2022, 7:00 AM
llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
230

I think R_RISCV_JAL don't need to left shift Value 12 bits.

jrtc27 requested changes to this revision.Jan 23 2022, 7:03 AM

Why is it so hard to go read the LLD code. This is clearly wrong. LLD:

case R_RISCV_JAL: {
  checkInt(loc, static_cast<int64_t>(val) >> 1, 20, rel);
  checkAlignment(loc, val, 2, rel);

  uint32_t insn = read32le(loc) & 0xFFF;
  uint32_t imm20 = extractBits(val, 20, 20) << 31;
  uint32_t imm10_1 = extractBits(val, 10, 1) << 21;
  uint32_t imm11 = extractBits(val, 11, 11) << 20;
  uint32_t imm19_12 = extractBits(val, 19, 12) << 12;
  insn |= imm20 | imm10_1 | imm11 | imm19_12;

  write32le(loc, insn);
  return;
}
This revision now requires changes to proceed.Jan 23 2022, 7:03 AM
fourdim updated this revision to Diff 402319.Jan 23 2022, 7:21 AM

address the comments and include the recent commits.

Sorry about arc diff issue. I will update it later.

fourdim updated this revision to Diff 402320.Jan 23 2022, 7:25 AM

fix the arc diff issue

jrtc27 added a comment.EditedJan 23 2022, 7:27 AM
This comment has been deleted.
llvm/include/llvm/ExecutionEngine/JITLink/riscv.h
47–51 ↗(On Diff #402320)

This is still all wrong

Whoops Phab did its weird thing of forgetting I had already submitted a comment so it got submitted again alongside my new comment

fourdim updated this revision to Diff 402323.Jan 23 2022, 7:31 AM
fourdim marked 2 inline comments as done.

Address the comments.

StephenFan accepted this revision.Feb 16 2022, 9:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2022, 7:03 AM
This revision was automatically updated to reflect the committed changes.
fourdim updated this revision to Diff 409839.Feb 17 2022, 7:16 PM

Include the recent commits and fix the test.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2022, 10:07 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.