Skip to content
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.

Sign up for GitHub

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

[SPARC][Driver] Add -m(no-)v8plus flags handling #98713

Merged
merged 2 commits into from
Aug 15, 2024
Merged

[SPARC][Driver] Add -m(no-)v8plus flags handling #98713

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea6720f
[SPARC][clang] Add -m(no-)v8plus flags handling
koachan Jul 13, 2024
d65ee3a
Change enabled feature bit (v9 -> v8plus)
koachan Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6077,6 +6077,10 @@ def mvis3 : Flag<["-"], "mvis3">, Group<m_sparc_Features_Group>;
def mno_vis3 : Flag<["-"], "mno-vis3">, Group<m_sparc_Features_Group>;
def mhard_quad_float : Flag<["-"], "mhard-quad-float">, Group<m_sparc_Features_Group>;
def msoft_quad_float : Flag<["-"], "msoft-quad-float">, Group<m_sparc_Features_Group>;
def mv8plus : Flag<["-"], "mv8plus">, Group<m_sparc_Features_Group>,
HelpText<"Enable V8+ mode, allowing use of 64-bit V9 instructions in 32-bit code">;
s-barannikov marked this conversation as resolved.
Show resolved Hide resolved
def mno_v8plus : Flag<["-"], "mno-v8plus">, Group<m_sparc_Features_Group>,
HelpText<"Disable V8+ mode">;
foreach i = 1 ... 7 in
def ffixed_g#i : Flag<["-"], "ffixed-g"#i>, Group<m_sparc_Features_Group>,
HelpText<"Reserve the G"#i#" register (SPARC only)">;
Expand Down
7 changes: 7 additions & 0 deletions clang/lib/Driver/ToolChains/Arch/Sparc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,13 @@ void sparc::getSparcTargetFeatures(const Driver &D, const ArgList &Args,
Features.push_back("-hard-quad-float");
}

if (Arg *A = Args.getLastArg(options::OPT_mv8plus, options::OPT_mno_v8plus)) {
if (A->getOption().matches(options::OPT_mv8plus))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually know whether a feature is default on or off.
If it is default off, the driver only passes +xxx, but suppresses -xxx.

Are both +v9 and -v9 needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is off by default, but we also want it to override features enabled implicitly by -mcpu flags.
For example if I pass -mcpu=v9 -mno-v8plus then I want the compilation to be done with v9 turned off.
So I think it is needed... unless there's other ways to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite confusing.
-m[no]-v8plus is an environment option, similar to X86's -mx32, but imposes more restrictions.
-mcpu=v9 -mno-v8plus should be equivalent to just -mcpu=v9 because 64-bit environment is the default for 64-bit ISA.
-mv8plus enables 32-bit environment and therefore should restrict the compiler from using the whole set of V9 features. That is, I would expect it to disable something rather than to enable v9, which should already be set by -mcpu=v9.

Copy link
Contributor

@s-barannikov s-barannikov Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, the order of -mv8plus relative to -mcpu shouldn't matter (one sets the environment, the other sets the ISA).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true that those flags primarily control the target environment, however, on GCC, -m[no-]v8plus also implies that V9 instructions are available (or not, in case of -mno-v8plus) (e.g when multiplying 64-bit numbers: https://godbolt.org/z/5zdWez6qz).
Also, unlike -mx32, -m[no-]v8plus is intended for use with 32-bit target (-m32); again, with GCC, -mv8plus raises an error with -m64, and -mno-v8plus is ignored (i.e it still generates 64-bit instructions) (https://godbolt.org/z/PcfMcT8cf).

It would be possible to define the flags such that -m[no-]v8plus do not touch the V9 feature bit, but I feel like that would defeat the purpose of V8+ environment (that is, to let 32-bit code use some 64-bit V9 instructions).

Copy link
Contributor

@s-barannikov s-barannikov Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on GCC, -m[no-]v8plus also implies that V9 instructions are available (or not, in case of -mno-v8plus)

Well, no. Not exactly. Whether or not V9 instructions are available is determined strictly by -mcpu=v9 (which is the default). In both cases by the first godbolt link instructions are V9 instructions and operate on 64-bit registers.
The thing is, you can't use the upper half of these registers reliably, which makes instructions such as MULX useless. There are, however, other instructions introduced in V9 that are usable -- 32-bit atomics for instance.

Also note that -mno-v8plus is the default and passing it explicitly won't change the behavior (unless you specified -mv8plus prior to it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.
In that case, would making these a no-op flag okay for now? As far as I understand it that would still be a compliant implementation, no? Codegen, etc. changes will happen in future patches but I can amend this one to include a placeholder v8plus feature bit in the backend.

What do you think about it?

(Some background: I am currently only looking to include this in clang because Linux passes -mv8plus when building 32-bit objects since some of its inline asm uses 64-bit registers, but otherwise it doesn't care if compiler-generated code uses them or not)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, would making these a no-op flag okay for now? As far as I understand it that would still be a compliant implementation, no?

AFAIK it is an ABI affecting flag, although I don't know if it changes anything except for ELF header's e_machine field compared to 32-bit V8/V9. It can't safely be ignored if we claim support for V8+. If we don't, and V8+ is otherwise compatible with 32-bit ABI, I think ignoring it and generating instructions should be fine, probably with a warning.

Codegen, etc. changes will happen in future patches but I can amend this one to include a placeholder v8plus feature bit in the backend.

What do you think about it?

I have little experience in target feature bits. It could be helpful to have a draft that adds some V8+ support to the backend and the corresponding feature bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK it is an ABI affecting flag, although I don't know if it changes anything except for ELF header's e_machine field compared to 32-bit V8/V9. It can't safely be ignored if we claim support for V8+. If we don't, and V8+ is otherwise compatible with 32-bit ABI, I think ignoring it and generating instructions should be fine, probably with a warning.

AFAICT the main ABI change is widening the G and O registers to 64-bits, which is compatible with legacy ABI:

  • The kernel handles saving and restoring those registers during context switches, so it is out of scope for us (aside from setting the correct ELF e_machine value so the kernel knows how much state to handle);
  • The (non-reserved) G registers are volatile anyway, so any code should not make assumptions about its value past a call or return;
  • The O registers are widened too, however, during a call, the callee might issue a save (which will result in the values being rotated - and truncated - into the I registers), so 64-bit arguments need to be split into 2x32-bit pair, just as in legacy ABI; and
  • Similarly, the callee might put its return value in the I registers before issuing a restore, so the caller will receive 64-bit returns as 2x32-bit pairs, just as in legacy ABI.

It could be helpful to have a draft that adds some V8+ support to the backend and the corresponding feature bits.

Backend bits are in #101367 (incl. ELF e_machine type setting), that should be merged first before this PR.

Features.push_back("+v9");
else
Features.push_back("-v9");
}

if (Args.hasArg(options::OPT_ffixed_g1))
Features.push_back("+reserve-g1");

Expand Down
5 changes: 5 additions & 0 deletions clang/test/Driver/sparc-target-features.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,8 @@
// RUN: %clang --target=sparc -msoft-quad-float %s -### 2>&1 | FileCheck -check-prefix=SOFT-QUAD-FLOAT %s
// HARD-QUAD-FLOAT: "-target-feature" "+hard-quad-float"
// SOFT-QUAD-FLOAT: "-target-feature" "-hard-quad-float"

// RUN: %clang --target=sparc -mv8plus %s -### 2>&1 | FileCheck -check-prefix=V8PLUS %s
// RUN: %clang --target=sparc -mno-v8plus %s -### 2>&1 | FileCheck -check-prefix=NO-V8PLUS %s
// V8PLUS: "-target-feature" "+v9"
// NO-V8PLUS: "-target-feature" "-v9"
Loading