This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] RISCV64 port
ClosedPublic

Authored by fpallares on Mar 27 2019, 8:03 AM.

Details

Summary

This is a port of libomp for the RISC-V 64-bit Linux target.

We have tested this port on a HiFive Unleashed development board using a downstream LLVM that has support for the missing bits in upstream. As of now, all tests are passing, including OMPT.

Diff Detail

Event Timeline

fpallares created this revision.Mar 27 2019, 8:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 8:03 AM

Most of the changes look pretty straight forward to me, but I currently don't have time to read through the assembly implementation of __kmp_invoke_microtask. I left two comments inline to discuss compressed vs uncompressed encodings for testing OMPT.

runtime/test/ompt/callback.h
170 ↗(On Diff #192441)

Should this also handle the case that the nop is uncompressed (this should be 4 bytes)?

172–173 ↗(On Diff #192441)

Did you try to find out why the compiler inserts a jump?
And, as above, should be also allow the instruction to be 4 bytes long?

fpallares updated this revision to Diff 192598.Mar 28 2019, 3:32 AM

Update print_possible_return_addresses macro in runtime/test/ompt/callback.h to handle uncompressed instructions, as pointed out by @Hahnfeld.

Thanks for the quick review @Hahnfeld, I've just replied to your inline comments and updated the patch!

runtime/test/ompt/callback.h
170 ↗(On Diff #192441)

As Linux requires riscv64gc we assumed compressed instructions would be enabled, but certainly it is not necessarily the case (riscv64g should be supported). Good catch!

172–173 ↗(On Diff #192441)

The unnecessary jump to the immediate successor block is removed by FastIsel in the targets that enable it. For now RISCV64 does not.

fpallares retitled this revision from [WIP] RISCV64 port to [OpenMP][WIP] RISCV64 port.Mar 29 2019, 4:42 AM
asb added a subscriber: asb.Jun 3 2019, 7:18 AM

Sorry for the delay...

runtime/src/z_Linux_asm.S
1720–1722 ↗(On Diff #192598)

I don't think we need this, it's now handled at all call-sites of __kmp_invoke_microtask. I'll post a patch to resolve any inconsistencies.

AFAICT removing this should reduce some of the (conditional) complexity of the stack layout.

1759–1760 ↗(On Diff #192598)

I'd propose to make them negative, the implementation for x86_64 does so as well.

1802–1805 ↗(On Diff #192598)

For example, this can go away.

1874–1878 ↗(On Diff #192598)

Not needed, as said above.

fpallares updated this revision to Diff 210595.Jul 18 2019, 9:15 AM

Remove reset of exit_frame pointer for OMPT since it is now done at call-site and fix return address computation macro used for some tests (as the generated code has changed).

fpallares marked 4 inline comments as done.Jul 18 2019, 9:25 AM

Thanks @Hahnfeld for the review. I've just updated the patch.

fpallares retitled this revision from [OpenMP][WIP] RISCV64 port to [OpenMP] RISCV64 port.Jul 18 2019, 9:26 AM
fpallares edited the summary of this revision. (Show Details)

Now that D60456 has landed we believe this need not be WIP anymore.

Hahnfeld accepted this revision.Jul 18 2019, 9:30 AM

The changes look good to me, thanks for your patience!

This revision is now accepted and ready to land.Jul 18 2019, 9:30 AM

I've tested this patch with current upstream LLVM and Clang on a HiFive Unleashed board and all tests are passing, including OMPT. I don't have commit access yet, @Hahnfeld would you mind commiting this for me? Thank you!

I've tested this patch with current upstream LLVM and Clang on a HiFive Unleashed board and all tests are passing, including OMPT. I don't have commit access yet, @Hahnfeld would you mind commiting this for me? Thank you!

Sure, not problem. The commit should be on its way through the repositories.

This revision was automatically updated to reflect the committed changes.