This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add Sparc support to Optimizer/CodeGen/Target.cpp
ClosedPublic

Authored by ro on Sep 9 2022, 1:41 AM.

Details

Summary

As described in Issue #57642, flang currently lacks SPARC support in Optimizer/CodeGen/Target.cpp, which causes a considerable number of tests to FAIL with

error: flang/lib/Optimizer/CodeGen/Target.cpp:310: not yet implemented: target not implemented

This patch fixes this by following GCC`s documentation of the ABI described in the Issue.

Tested on sparcv9-sun-solaris2.11.

I'm not fully certain I've done this correctly, so extra scrutiny would be most welcome. Besides, I wonder why flang has to do this at all (and for complex values only) since the SPARC backend knows how to handle complex args and return values just fine.

Diff Detail

Repository
rG LLVM Github Monorepo
Build Status
Buildable 188153

Event Timeline

ro created this revision.Sep 9 2022, 1:41 AM
ro requested review of this revision.Sep 9 2022, 1:41 AM
rovka added a comment.Sep 15 2022, 7:06 AM

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

ro updated this revision to Diff 462148.Sep 22 2022, 5:36 AM

Add testcase.

ro added a comment.Sep 22 2022, 5:38 AM

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

The new testcase PASSes on sparcv9-sun-solaris2.11 and sparc64-unknown-linux-gnu. However, for 32-bit sparc it runs into an assertion failure

Assertion failed: llvm::all_of(types, [](Type t) { return t; }) && "attempting to construct a TypeRange with null types", file /vol/llvm/src/llvm-project/local/mlir/lib/IR/TypeRange.cpp, line 20

and I don't yet see what's wrong. Any suggestions or do I need to run a full Debug build to investigate?

rovka accepted this revision.Sep 27 2022, 2:51 AM
In D133561#3808337, @ro wrote:

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

The new testcase PASSes on sparcv9-sun-solaris2.11 and sparc64-unknown-linux-gnu. However, for 32-bit sparc it runs into an assertion failure

Assertion failed: llvm::all_of(types, [](Type t) { return t; }) && "attempting to construct a TypeRange with null types", file /vol/llvm/src/llvm-project/local/mlir/lib/IR/TypeRange.cpp, line 20

and I don't yet see what's wrong. Any suggestions or do I need to run a full Debug build to investigate?

Not sure what the issue is there. I'm actually a bit surprised that it works for i386. When I tried to enable flang for armv7, MLIR didn't have very good support for 32-bit platforms, so I left it at that. If you know that users of 32-bit sparc really have a great need for a Fortran compiler, then feel free to investigate further, but I'm not sure how many other 32-bit platforms have an interest in flang/mlir, so maintaining it might turn out to be a non-trivial amount of work.

Otherwise, thanks for the test, this LGTM now. However I'm not familiar with the SPARC ABI - do you know anyone that usually reviews SPARC patches and could have a look at this before it gets committed?

flang/test/Fir/target-rewrite-complex.fir
5

Nit: It would be nice to also test a Linux triple here (it can use the same check prefix as Solaris).

This revision is now accepted and ready to land.Sep 27 2022, 2:51 AM
ro marked an inline comment as done.Sep 27 2022, 5:24 AM
In D133561#3808337, @ro wrote:

This doesn't have any tests, could you please add some? If memory holds, you should be able to add some RUN/CHECK lines here: https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/target-rewrite-complex.fir

The new testcase PASSes on sparcv9-sun-solaris2.11 and sparc64-unknown-linux-gnu. However, for 32-bit sparc it runs into an assertion failure

[...]

Not sure what the issue is there. I'm actually a bit surprised that it works for i386. When I tried to enable flang for armv7, MLIR didn't have very good support for 32-bit platforms, so I left it at that. If you know that users of 32-bit sparc really have a great need for a Fortran compiler, then feel free to investigate further, but I'm not sure how many other 32-bit platforms have an interest in flang/mlir, so maintaining it might turn out to be a non-trivial amount of work.

I don't think there will be great need for 32-bit support, so I'll leave this 64-bit only for now. Maybe others will have the inclination to dive further and sparc can benefit ;-)

Otherwise, thanks for the test, this LGTM now. However I'm not familiar with the SPARC ABI - do you know anyone that usually reviews SPARC patches and could have a look at this before it gets committed?

I've added some of the usual suspects as reviewers. However, given that SPARC lacks maintainers in LLVM, that's always been difficult.

flang/test/Fir/target-rewrite-complex.fir
5

Will do. It should be helpful because Solaris and Linux differ in their long double type, although LLVM is pretty broken in that respect right now.

ro updated this revision to Diff 463199.Sep 27 2022, 5:33 AM
ro marked an inline comment as done.

Run test on Linux/sparc64, too.

This revision was automatically updated to reflect the committed changes.
ro added a comment.Oct 3 2022, 5:05 AM

Given that there were no comments from the SPARC reviewers in more than a week, I've now committed the patch as is.