[RFC] Multilib

petrhosek · 2023-03-15T06:39:47.946Z

mplatings:

Thanks Petr for adding your reviews quickly. I’ve responded to all of them except the ones on D142933 which I’ll get to tomorrow.

Thanks, I’ll try to do another pass over your changes tomorrow.

mplatings:

Yes you can do this:

Variants:
  - Dir: itanium
    Flags: [fc++abi=itanium]
    PrintOptions: [-fc++abi=itanium]

Do you think it’d be possible to derive PrintOptions automatically (at least for the simple cases such as this one) to avoid the duplication?

I’ve also noticed that a lot of the complexity in the new implementation is due to -print-multi-lib support (such as needing the PrintOptions field) which makes me wonder whether there’s a reason why this is needed? Is the goal to support existing tools that consume the output of -print-multi-lib? If not, could we provide a different option with a simpler output that doesn’t require all this complexity?

mplatings · 2023-03-15T10:26:13.412Z

petrhosek:

Do you think it’d be possible to derive PrintOptions automatically (at least for the simple cases such as this one) to avoid the duplication?

It’s tempting but I think the cost will be an extra wrinkle in the API. In practice I expect most multilib.yaml files to be generated rather than hand-written, in which case the duplication is less of an issue and keeping the API simpler will be preferable. This prediction may prove incorrect but the best test of that is getting people more people actively using the system. Therefore this question is one I’d like to explore after landing the current stack of patches.

petrhosek:

I’ve also noticed that a lot of the complexity in the new implementation is due to -print-multi-lib support (such as needing the PrintOptions field) which makes me wonder whether there’s a reason why this is needed? Is the goal to support existing tools that consume the output of -print-multi-lib? If not, could we provide a different option with a simpler output that doesn’t require all this complexity?

Yes the picolibc build system does interrogate -print-multi-lib output. Also I feel we need to continue to provide -print-multi-lib functionality because it’s an API that clang shares with gcc.
I share your desire to keep things simple and fast. My latest update to ⚙ D142905 [Driver] Change multilib selection algorithm improved speed at the expense of increasing complexity slightly so I’ll take another look to see if we can have speed without compromising on simplicity.

mplatings · 2023-03-15T23:02:03.752Z

mplatings:

My latest update to D142905 [Driver] Change multilib selection algorithm improved speed at the expense of increasing complexity slightly so I’ll take another look to see if we can have speed without compromising on simplicity.

I think we have to pick our poison:

Have the callback function to enable lazily calculating the print options.
Add plumbing to many functions so we can pass a NeedPrintOptions parameter to MultilibBuilder.makeMultilib() to tell it whether or not to calculate the print options at that time.
Eat the 2 microsecond cost of calculating the print options every time you invoke clang.

Right now we have option 1 and that particular poison doesn’t taste too bad to me.

I’ve now responded to your comments on D142933. Over to you!

jroelofs · 2023-03-16T01:00:43.874Z

mplatings:

An example of backwards compatibility:
If I’m building my application with -mfloat-abi=softfp then I can use a library built with either -mfloat-abi=softfp or -mfloat-abi=soft. The ABI is the same in both cases but the former requires an FPU. softfp is backwards compatible with soft.
However if I’m building my application with -mfloat-abi=soft then a multilib system must assume that an FPU isn’t available and must only link with a library that was also built with -mfloat-abi=soft.
This directional compatibility can’t be expressed with Clang’s existing Multilib class.

Sorry I’m late to the thread but AFAIU, Multilib::flag() can express this specific case already:

auto SoftFP = MultilibBuilder(“/soft”,“”,“”,0)
.flag(“+mfloat-abi=softfp”)
.flag(“+mfloat-abi=soft”);
auto HardFP = MultilibBuilder(“/hard”,“”,“”,1)
.flag(“+mfloat-abi=hard”)
.flag(“+mfloat-abi=softfp”)
.flag(“-mfloat-abi=soft”);
MultilibSet Multilibs = MultilibSet().Either(SoftFp, HardFp);

If you build with -mfloat-abi=hard, you should get the /hard multilib.
If you build with -mfloat-abi=softfp, both are valid and the /hard multilib is selected because it has higher priority.
If you build with -mfloat-abi=soft, you should get the /soft multilib.

mplatings · 2023-03-16T13:09:36.798Z

Hi Jon, I think when I wrote that I was overly attached to the idea that you should use addMultilibFlag. If you always use addMultilibFlag then that case cannot work, but get a little more creative and I agree you can express it:

auto Soft = MultilibBuilder("/soft","","",0)
  .flag("+mfloat-abi=soft");
auto SoftFP = MultilibBuilder("/softfp","","",1)
  .flag("+mfloat-abi=softfp");
auto Hard = MultilibBuilder("/hard","","",2)
  .flag("+mfloat-abi=hard");
MultilibSet Multilibs = MultilibSet().Either(Soft, SoftFP, Hard);
if (ABI == arm::FloatABI::Soft) {
  Flags.push_back("+mfloat-abi=soft");
  Flags.push_back("-mfloat-abi=softfp");
  Flags.push_back("-mfloat-abi=hard");
}
if (ABI == arm::FloatABI::SoftFP) {
  Flags.push_back("+mfloat-abi=soft");
  Flags.push_back("+mfloat-abi=softfp");
  Flags.push_back("-mfloat-abi=hard");
}
if (ABI == arm::FloatABI::Hard) {
  Flags.push_back("-mfloat-abi=soft");
  Flags.push_back("-mfloat-abi=softfp");
  Flags.push_back("+mfloat-abi=hard");
}

I am aiming to change the selection criterion to “a multilib is compatible its flags are a subset of the flags derived from the command line arguments” and then the code gets smaller:

auto Soft = MultilibBuilder("/soft","","")
  .flag("+mfloat-abi=soft");
auto SoftFP = MultilibBuilder("/softfp","","")
  .flag("+mfloat-abi=softfp");
auto HardFP = MultilibBuilder("/hard","","")
  .flag("+mfloat-abi=hard");
MultilibSet Multilibs = MultilibSet().Either(Soft, SoftFp, HardFp);
if (ABI == arm::FloatABI::Soft)
  Flags.push_back("+mfloat-abi=soft");
if (ABI == arm::FloatABI::SoftFP) {
  Flags.push_back("+mfloat-abi=soft");
  Flags.push_back("+mfloat-abi=softfp");
}
if (ABI == arm::FloatABI::Hard)
  Flags.push_back("+mfloat-abi=hard");

mplatings · 2023-04-17T12:11:45.252Z

We’ve gone live with a version 0.1 of the multilib format in LLVM Embedded Toolchain for Arm 16.0.0. Feedback is very welcome either here or in the GitHub issues.

petrhosek · 2023-04-21T22:04:50.693Z

Thanks, seeing a real-world multilib.yaml example is actually very helpful (including the logic that was used to generate it). The part I’m still unsure about is why do we need PrintOptions separate from Flags. This aspect is adding a lot of implementation and configuration complexity, and it’s not clear to me why it’s needed. Can you explain the motivation? Why can’t we use Flags for the -print-multi-lib output?

mplatings · 2023-04-25T15:29:27.027Z

Hi Petr,

The main problem I’m trying to work around here is ARM Extensions and AArch64 feature modifiers.

Unlike regular options like -fno-exceptions and -mfloat-abi which exist as independent options, all the architecture extensions are crushed into a single option e.g. -march=armv8.1-m.main+dsp+mve.fp+fp.dp+cdecp0+pacbti and each extension can be negated by prefixing it with no.

This is not ideal for multilib. When writing a multilib configuration I want to be able to express “this library requires the DSP extension and the PACBTI extension”. Currently the code in my patches allows me to add march=+dsp and march=+pacbti to multilib.yaml to do that. Neither flag is a valid command line option, and currently that’s OK.

What if I were to change the multilib system to require that flags are valid command line options?

Instead of generating march=+thing flags, a valid normalized -march option would have to be generated, for example -march=thumbv8.1m.main+dsp+mve+fp16+pacbti.
Each library in a multilib configuration would need a regular expression to match any suitable -march option and map it to a specific option e.g.

- Regex: -march=thumbv8\.[1-9]m\.main.*\+dsp.*\+pacbti
  MatchFlags: [-march=thumbv8.1m.main+dsp+pacbti]

It’s possible to get this to work, but it’s going to be more error-prone than directly stating the required extensions. It’s also more brittle. Let’s say in a later version of Clang that we change how the architecture extension list is normalised, and we append disabled options to the generated -march option. Now the regular expression in the multilib configuration is broken because we forgot .* at the end. Or maybe the extensions get reordered and now pacbti preceeds dsp. etc.

I suspect this problem will also crop up on RISC-V since that also has ISA extensions concatenated into a single -march string.

However, if it helps move things forwards, I can go the route of generating a normalized -march argument as described above, drop PrintOptions for now, and we can revisit this issue later.

What do you think?

petrhosek · 2023-04-27T07:11:56.990Z

Thanks for the additional context Michael, that’s really helpful. I’d also suggest including some of this in the documentation since it might be helpful in understanding the design.

mplatings:

What if I were to change the multilib system to require that flags are valid command line options?

Instead of generating march=+thing flags, a valid normalized -march option would have to be generated, for example -march=thumbv8.1m.main+dsp+mve+fp16+pacbti.

Each library in a multilib configuration would need a regular expression to match any suitable -march option and map it to a specific option e.g.
- Regex: -march=thumbv8\.[1-9]m\.main.*\+dsp.*\+pacbti
  MatchFlags: [-march=thumbv8.1m.main+dsp+pacbti]
It’s possible to get this to work, but it’s going to be more error-prone than directly stating the required extensions. It’s also more brittle. Let’s say in a later version of Clang that we change how the architecture extension list is normalised, and we append disabled options to the generated -march option. Now the regular expression in the multilib configuration is broken because we forgot .* at the end. Or maybe the extensions get reordered and now pacbti preceeds dsp. etc.

However, if it helps move things forwards, I can go the route of generating a normalized -march argument as described above, drop PrintOptions for now, and we can revisit this issue later.

What do you think?

I like this direction since it would let us land the rest of the stack and then us experiment with other possible approaches.

I agree that relying solely on regular expressions is potentially error-prone, but there might be other ways to handle this case in the flag mapping. For example, this problem reminds me of minion matching in Salt; to handle more complex scenarios Salt supports compound matchers and I wonder if we could use a similar approach here (to express your example you could use something like E@-march=thumbv8\.[1-9]m and M@feature:pacbti and not M@feature:pacbti)? This is just one idea, there might be other solutions.

mplatings · 2023-05-03T10:35:20.548Z

Hi Petr, I’ve gone ahead and updated the code to generate a normalised -march option and removed the separation of “PrintOptions”. Looking forward to your thoughts on https://reviews.llvm.org/D146757 et al.

mplatings · 2023-06-19T13:39:29.959Z

The multilib changes were landed last Wednesday and all seems well. Thanks @petrhosek for approving the changes. It was a long slog so thanks for keeping going with it.

LLVM Embedded Toolchain for Arm now has multilib support in its main branch. I’ll be interested to see if/how the configurable multilib can be used in other projects.

The feedback we’ve had so far is that it’s very hard to debug multilib when the user’s options don’t match any multilib variant. Therefore I’m working on making the multilib system emit an error message in this case, to hopefully direct the user to being able to find a suitable set of compile flags. I should have a patch up for review soon, and I hope to also get it in before LLVM 17 branching begins. Edit: patch is ⚙ D153292 [Driver][BareMetal] Error if no matching multilib

widlarizer · 2023-12-12T13:13:09.561Z

Hello Michael, could you elaborate on the exceptions use case? Let me know if my understanding is correct:

only flags built by ToolChain::getMultilibFlags are available for matching in yaml
getARMMultilibFlags doesn’t map -fexceptions or -fno-exceptions
therefore there is no mechanism for the use case you describe in clang/docs/Multilib.rst

So I’m struggling to find an interface for me to get the exception status to add to my multilib flags Result vector to compose an experimental embedded RISC-V toolchain. Any ideas on how to proceed?

mplatings · 2023-12-12T13:41:15.997Z

Hi @widlarizer, I think your understanding is correct.
An earlier version of getMultiSelection flags did have exceptions support: ⚙ D142933 [Driver] Add -print-multi-flags-experimental option

  // Enumerate boolean flags we care about for the purposes of multilib here.
  // There must be a smarter way to do it but this gets us started.
  const struct HasFlag {
    ID Pos, Neg;
    bool Default;
  } HasFlagList[] = {
      {OPT_fexceptions, OPT_fno_exceptions, true},
      {OPT_frtti, OPT_fno_rtti, true},
  };

  for (const HasFlag &HF : HasFlagList) {
    ID Option = Args.hasFlag(HF.Pos, HF.Neg, HF.Default) ? HF.Pos : HF.Neg;
    Result.push_back(
        "-" + clang::driver::getDriverOptTable().getOptionName(Option).str());
  }

That functionality was removed from the patch:

@petrhosek Jun 7 2023: I’d suggest omitting these for the initial version. Ideally we would find a way to specify these more generally through Clang’s option parser rather than individually listing all flags here, but that can be done in a follow up change.

If you’re interested in adding that functionality then you’re very welcome to do so.

widlarizer · 2023-12-12T14:29:50.417Z

I see. Did Petr mean to suggest that the list of flags to be considered in multilib selection should be hardcoded elsewhere, or somehow dynamic from the command line invocation? Various toolchains will no doubt want to differentiate multilibs based on different priorities, depending on the product line they are meant to support. This, to me, suggests that it should be a compilation time choice, similar to TARGETS_TO_BUILD.

Here’s what I have so far: [RISCV][RFC] BareMetal multilibs YAML usage by ekliptik · Pull Request #75191 · llvm/llvm-project · GitHub

pwprzybyla · 2023-12-12T14:32:25.839Z

Hi there is a PR that address multilib selection of libraries with and without exceptions:
Multilib selection with/without exceptions.

petrhosek · 2023-12-13T08:22:46.253Z

I think that we should support the -f[no-]exception and -f[no-]rtti cases for multilib selection. The question is what other flags should be supported as well and how to handle them in maintainable way. For example, I think we should also support -fsanitize since I’m aware of more than one toolchain that ships multilib variants for sanitizer instrumented runtime libraries. I also expect the list of flags to grow over time.

An idea I had is to add a new Clang options flag which would be used to specify which flags should be considered for multilib selection, but I haven’t yet had time to prototype this approach to see whether it’s feasible or not.

widlarizer · 2023-12-13T18:02:16.985Z

It would probably accommodate all toolchain vendors’ needs if we passed all flags. Is this feasible? Would it be too error-prone due to false-positive matches? In getARMMultilibFlags I added:

+  ArgStringList ASL;
+  for (const auto &A : Args) {
+    A->render(Args, ASL);
+  }
+  for (const auto &A : ASL) {
+    Result.push_back(A);
+  }
+  for (const auto &Arg : ASL)
+    Result.push_back(Arg);

hyperfine "./bin/llvm-lit ../clang/test/Driver/baremetal-multilib.yaml" -m 100
without patch: 115.0 ms ± 5.3 ms
with patch: 115.5 ms ± 3.0 ms

widlarizer · 2023-12-15T23:36:41.806Z

One thing that is not clear to me: is the YAML setup meant to replace hard-coded matching logic? It seems to have done so. Except for RISC-V which has special handling, I assume to prevent breakage in some external project.

mplatings · 2024-01-02T11:54:16.154Z

widlarizer:

It would probably accommodate all toolchain vendors’ needs if we passed all flags

Please don’t do that. Flags are way more complex than they seem. Flags can override each other, one flag can imply another flag, flags with different names can mean the same thing or slightly different things.

Flags should be changed to a normalized form before reaching the YAML matching system. That’s what getARMMultilibFlags does, and RISC-V and other toolchains will need something similar.

widlarizer:

is the YAML setup meant to replace hard-coded matching logic? It seems to have done so. Except for RISC-V which has special handling

Yes, the YAML setup is meant to replace hard-coded matching logic. The hard-coded RISC-V matching logic predates the YAML system.

widlarizer · 2024-01-03T09:47:30.045Z

Understood, thanks. I got it to a working state and it fits our use case well. Except for one detail. An embedded toolchain that supports a list of -mcpu values has no “magic” default value. When -march or -mcpu isn’t specified, the compiler should be unusable through some kind of error. What happens currently is that it falls back to RV32IM (or something similar), emits a warning about missing multilibs, and obviously LLD errors out. The problem is this warning is emitted even when you run --version. This shows that the multilib selection expects a valid multilib for the default ISA. We don’t want to ship that multilib and we don’t want to change the default ISA to a shipped one.

Should I consider this a bug and try build a patch for it?