• Rebase
• Improve ABS symbol handling:
  • Reuse existing symbol-lookup code when handling relocations so we don't need to create new undefined symbols.
  • Also handle ABS symbol relocation if their value happens to point to a section.

Hi all, I just got the following comment on the RuntimeDyLd part of this patch:

Friendly ping.

Fix failing test, sorry about that!

• Add RISCVISAInfo unit tests
• Update error message of RISCVISAInfo::parseArchString to reflect that we support RV64E now

In D143570#4216040, @asb wrote:

I expect you'll need to / want to update the C++ RISCVISAInfo unit tests as they were added after this patch was first proposed.

Woops, sorry about that. Done!

In D143570#4215828, @kito-cheng wrote:

@jobnoorman do you need some help to commit this and D144384? let me know if you need help, and I would suggest you could drop a mail to Chris to obtain write access https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access :)

So it feels to me (please correct me if I'm wrong) that there is some agreement that the current patch is the preferred approach for dealing with the composed relocations in RISC-V. There might be a need though for supporting custom MCExprs in relocations for features found in golang. I think it should be possible to implement that *in addition to* the current patch so maybe it would make sense to move that discussion to another RFC?

In D146546#4212407, @treapster wrote:

The implemented feature isn't used (afaict) by X86 or AArch64 so this should in principle be NFC

In fact we need to have it for other targets because for example some golang structures contain offsets into .text which is ok if we know output .text address, but if we land https://reviews.llvm.org/D144560 it would be cumbersume to reserve .text address ahead of time, and i think other cases of symbol diffs or other expressions may emerge outside of golang. Last week i tried to merge golang with -rewrite and ended up creating bogus relocation type which can contain arbitrary symbol expression and has advantages of not using multiset and extra indirection at the cost of Relocation's PODness and privatizing Relocation::Symbol. However, in case of RISCV we'll need to collect composed relocations in a single expression while reading them and only then add them to BinarySection. Here's the patch, should i open a competing RFC with it?:)

0001-BOLT-Use-raw-MCExpr-to-express-symbol-differences.patch16 KBDownload

In D146546#4210355, @yota9 wrote:

Nice! I didn't know about this feature. May I ask you to create dependent review with getComposeOpcodeFor ? Thank you!

Thanks for reviewing @yota9, @rafauler! Please note the I don't have commit access so I cannot land this myself.

In D146215#4202973, @rafauler wrote:

Makes sense, but where did you see this in practice?

Swap check with the if (!Section->isText()) check as suggested by @yota9.

Friendly ping.

Rebase and address reviewer comments:

• Adapt to D131813 and D145972
• Remove duplicated method getOrCreateAbsoluteSymbol
• Do not depend on libBOLTCore in libBOLTTarget: have the pass loop over instructions and only call back to MCPlusBuilder for target-specific info
• Update bolt/CODE_OWNERS.TXT

Address some of the reviewer comments:

• Remove references to PCREL_LO12_S. It was only partially implemented so I'll keep the full implementation for a later patch;
• Collapse two separate ifs in one;
• Invoke call-fixing code from binary pass;
• Detect c.nop in isNoop()

Job Noorman <jnoorman@igalia.com>

Thanks for the review, @Amir! I'm not sure if one accept is enough to get this landed but if it is, please note that I don't have commit rights so I cannot do it myself.

Fix failing test.

Rebase

Ping

Allow rv64e in arch strings (e.g., in the arch assembly attribute). Also add a test for this.

Ping.

Rebase.

Sure! Job Noorman <jnoorman@igalia.com>

Thanks for the review, @kito-cheng. Note that I do not have commit access so I cannot commit this myself.

Fix patch apply error. Sorry about the noise.

Address reviewer comments:

• Sidestep stack alignment uncertainty by using report_fatal_error when codegen is reached for RVE;
• Unify codegen error reporting for RV32E and RV64E;
• Update RISCVUsage.rst and ReleaseNotes.rst

Address comments:

• Remove setting location of generated addi
• Mask high bits of fixup value
• Add test for error case

In D135960#4111113, @jrtc27 wrote:

This seems rather limited in use...

Fix Clang driver tests.

Rebase on current main.

Thanks for the review!

This patch should be committed in parallel with a similar patch for Clang. Can I just do that or should a create a diff for that one too?

This is the same patch but based on current trunk.

This is the same patch but based on current trunk.

Yes, I think we should do this (maybe even drop matching on the node name completely?). Once I have some time, I'll try to prepare a patch and post it for review.

Yes, that took me by surprise as well :-)

Thanks for the reviews! r216716