Use LLVM New Pass Manager #319

Merged
sb10q merged 1 commits from llvm-new-pass-manager into master 2024-08-17 17:37:20 +08:00
Collaborator

This PR refactors the codebase to use LLVM's New Pass Manager. The major change involves passing the TargetMachine information around, as the New Pass Manager also requires an instance of TargetMachine to perform optimizations.

This refactoring also adds 3 new command-line parameters: --target, --mcpu, and --target-features, which can be used to specify cross-compilation options. This has been manually tested using ./run_demo.sh --mcpu=x86-64-v3 src/lists.py and ./run_demo.sh --mcpu=x86-64-v3 src/lists.py, which results in --mcpu=x86-64-v3 generating AVX2 instructions whereas --mcpu=x86-64 would only generate SSE2 instructions. --mcpu also supports native, i.e. optimizing for the host machine, similar to other C compilers.

Some questions before this MR is ready:

  1. In the nac3artiq module, the cpu argument of target.create_target_machine is set to "". Would it be better if we change this to the specific CPU names, e.g. cortex-a9 for Isa::CortexA9, generic-rv32 for Isa::RiscV32* etc.

Depends on #318 to be merged first.

This PR refactors the codebase to use LLVM's [New Pass Manager](https://llvm.org/docs/NewPassManager.html). The major change involves passing the `TargetMachine` information around, as the New Pass Manager also requires an instance of `TargetMachine` to perform optimizations. This refactoring also adds 3 new command-line parameters: `--target`, `--mcpu`, and `--target-features`, which can be used to specify cross-compilation options. This has been manually tested using `./run_demo.sh --mcpu=x86-64-v3 src/lists.py` and `./run_demo.sh --mcpu=x86-64-v3 src/lists.py`, which results in `--mcpu=x86-64-v3` generating AVX2 instructions whereas `--mcpu=x86-64` would only generate SSE2 instructions. `--mcpu` also supports `native`, i.e. optimizing for the host machine, similar to other C compilers. Some questions before this MR is ready: 1. In the `nac3artiq` module, the `cpu` argument of `target.create_target_machine` is set to `""`. Would it be better if we change this to the specific CPU names, e.g. `cortex-a9` for `Isa::CortexA9`, `generic-rv32` for `Isa::RiscV32*` etc. Depends on #318 to be merged first.
derppening self-assigned this 2023-09-12 15:34:35 +08:00
derppening added 3 commits 2023-09-12 15:34:35 +08:00
In preparation for adding more command-line options.
For specifying LLVM options during code generation.
Owner

In the nac3artiq module, the cpu argument of target.create_target_machine is set to "". Would it be better if we change this to the specific CPU names, e.g. cortex-a9 for Isa::CortexA9, generic-rv32 for Isa::RiscV32* etc.

Do you understand why it was set to ""? What are the consequences of this proposed change?

> In the nac3artiq module, the cpu argument of target.create_target_machine is set to "". Would it be better if we change this to the specific CPU names, e.g. cortex-a9 for Isa::CortexA9, generic-rv32 for Isa::RiscV32* etc. Do you understand why it was set to ""? What are the consequences of this proposed change?
Author
Collaborator

Do you understand why it was set to ""?

Not really. git blame appears to show that this change was made by you, so maybe you would have an idea?

What are the consequences of this proposed change?

My understanding is that this change should not impact anything, assuming that the compiler only targets Cortex-A9 or any Generic RISC-V32 devices, as the CPU string would match the CPU it is compiling for. A benefit would be that the entire optimization pipeline and backend would optimize for this specific chip, meaning that it could bring performance improvements.

However, if someone wants to use nac3artiq for other ARM chips, then this may or may not work depending on whether the chip supports a superset of instructions. We may also implement generic profiles for RISC-V32 and ARMv7 to maintain compatibility for these kinds of use-cases.

> Do you understand why it was set to ""? Not really. `git blame` appears to show that this change was made by you, so maybe you would have an idea? > What are the consequences of this proposed change? My understanding is that this change should not impact anything, assuming that the compiler only targets Cortex-A9 or any Generic RISC-V32 devices, as the CPU string would match the CPU it is compiling for. A benefit would be that the entire optimization pipeline and backend would optimize for this specific chip, meaning that it could bring performance improvements. However, if someone wants to use `nac3artiq` for other ARM chips, then this may or may not work depending on whether the chip supports a superset of instructions. We may also implement generic profiles for RISC-V32 and ARMv7 to maintain compatibility for these kinds of use-cases.
derppening force-pushed llvm-new-pass-manager from 326ed52324 to 966ce7a659 2023-09-12 18:51:25 +08:00 Compare
Author
Collaborator

v2: Rebased against master.

v2: Rebased against master.
Owner

Ah, I see. Using the specific CPU names sounds good, we know what devices need to be supported by nac3artiq. I'm not sure if we have a "generic RISC-V" since we support things like hardware FPU on some platforms - check the legacy compiler for the correct flags/CPU model to put there.

Ah, I see. Using the specific CPU names sounds good, we know what devices need to be supported by nac3artiq. I'm not sure if we have a "generic RISC-V" since we support things like hardware FPU on some platforms - check the legacy compiler for the correct flags/CPU model to put there.
derppening force-pushed llvm-new-pass-manager from 966ce7a659 to 6f9c464230 2023-09-14 14:11:49 +08:00 Compare
Author
Collaborator

v3: Rebased against master, added target CPU name to TargetMachine creation, and updated check_demos.sh script to allow arguments to the compiler.

The PR is ready for review.

v3: Rebased against master, added target CPU name to `TargetMachine` creation, and updated `check_demos.sh` script to allow arguments to the compiler. The PR is ready for review.
derppening changed title from WIP: Use LLVM New Pass Manager to Use LLVM New Pass Manager 2023-09-14 14:13:17 +08:00
derppening requested review from sb10q 2023-09-14 14:13:21 +08:00
derppening changed title from Use LLVM New Pass Manager to WIP: Use LLVM New Pass Manager 2023-09-14 14:14:25 +08:00
Author
Collaborator

On second thought - I think it's better if I implement a flag which can toggle between the new and old pass managers, since there are still outstanding bugs we need to fix when building using the old pass manager.

On second thought - I think it's better if I implement a flag which can toggle between the new and old pass managers, since there are still outstanding bugs we need to fix when building using the old pass manager.
derppening force-pushed llvm-new-pass-manager from 6f9c464230 to 667ebc699c 2023-09-14 15:36:32 +08:00 Compare
Author
Collaborator

v4: Changed implementation to keep both legacy and new PMs for testing, refactored codegen test cases to test both legacy and new PMs.

Ready for review.

v4: Changed implementation to keep both legacy and new PMs for testing, refactored codegen test cases to test both legacy and new PMs. Ready for review.
derppening changed title from WIP: Use LLVM New Pass Manager to Use LLVM New Pass Manager 2023-09-14 15:38:34 +08:00
sb10q reviewed 2023-09-14 17:31:44 +08:00
@ -66,1 +67,4 @@
pub opt_level: OptimizationLevel,
/// Whether to use LLVM's Legacy Pass Manager.
pub legacy_pm: bool,
Owner

Why would we?

Why would we?
Author
Collaborator

Mostly to diagnose problems that occur only using the Legacy PM, e.g. #315.

Mostly to diagnose problems that occur only using the Legacy PM, e.g. #315.
sb10q reviewed 2023-09-14 17:32:42 +08:00
@ -894,6 +926,9 @@ impl Nac3 {
deferred_eval_store: DeferredEvaluationStore::new(),
llvm_options: CodeGenLLVMOptions {
opt_level: OptimizationLevel::Default,
// FIXME(Derppening): Add a field to device_db.py for modifying this option
Owner

Which option? Also it's better to open issues than write fixme comments.

Which option? Also it's better to open issues than write fixme comments.
Author
Collaborator

In a sense it is probably not necessary to expose the legacy PM to users of ARTIQ, so this change is removed until someone else thinks it's necessary.

In a sense it is probably not necessary to expose the legacy PM to users of ARTIQ, so this change is removed until someone else thinks it's necessary.
derppening force-pushed llvm-new-pass-manager from 667ebc699c to 96f940f16c 2023-09-15 10:12:28 +08:00 Compare
Author
Collaborator

v5: Fixed incorrect CPU name for RISC-V.
v6: Removed FIXME

v5: Fixed incorrect CPU name for RISC-V. v6: Removed FIXME
derppening force-pushed llvm-new-pass-manager from 96f940f16c to 47c507453c 2023-09-15 10:16:52 +08:00 Compare
sb10q reviewed 2023-09-15 16:47:59 +08:00
@ -663,0 +658,4 @@
.create_target_machine(self.llvm_options.opt_level)
.expect("couldn't create target machine");
if self.llvm_options.legacy_pm {
Owner

legacy_pm still needs to be removed. I don't see a use case for this, except perhaps tackling #315, but you could just patch your local copy while bug-hunting.

legacy_pm still needs to be removed. I don't see a use case for this, except perhaps tackling https://git.m-labs.hk/M-Labs/nac3/issues/315, but you could just patch your local copy while bug-hunting.
sb10q reviewed 2023-09-15 16:48:03 +08:00
sb10q reviewed 2023-09-15 16:48:03 +08:00
derppening force-pushed llvm-new-pass-manager from 47c507453c to 443f7d37e1 2023-09-18 09:55:46 +08:00 Compare
Author
Collaborator

v7: Rebased against master, removed legacy_pm configuration

v7: Rebased against master, removed `legacy_pm` configuration
sb10q reviewed 2023-09-18 10:18:05 +08:00
@ -663,0 +663,4 @@
let result = main.run_passes("default<O3>", &target_machine, pass_options);
if let Err(err) = result {
println!("Failed to run optimization for module `main`");
println!("{}", err.to_string());
Owner

Why not just move these println into the panic message?

Why not just move these println into the panic message?
sb10q reviewed 2023-09-18 10:18:39 +08:00
@ -282,0 +366,4 @@
if let Err(err) = result {
println!("Failed to run optimization for module `{}`", module.get_name().to_str().unwrap());
println!("{}", err.to_string());
panic!();
Owner

same here

same here
sb10q reviewed 2023-09-18 10:18:54 +08:00
@ -345,0 +366,4 @@
let result = main.run_passes("default<O3>", &target_machine, pass_options);
if let Err(err) = result {
println!("Failed to run optimization for module `main`");
println!("{}", err.to_string());
Owner

and here

and here
derppening force-pushed llvm-new-pass-manager from 443f7d37e1 to 50703e515d 2023-09-18 11:26:06 +08:00 Compare
derppening force-pushed llvm-new-pass-manager from 50703e515d to d5fb0ddfd9 2023-09-18 11:29:47 +08:00 Compare
Author
Collaborator

v8, v9: Fixed compilation error with 3c2696c15c, moved message into panic!

v8, v9: Fixed compilation error with 3c2696c15c, moved message into `panic!`
sb10q reviewed 2023-09-18 11:30:48 +08:00
@ -663,0 +662,4 @@
pass_options.set_merge_functions(true);
let result = main.run_passes("default<O3>", &target_machine, pass_options);
if let Err(err) = result {
panic!("Failed to run optimization for module `main`\n{}", err.to_string());
Owner

Is it common or recommended practice to have \n in panic messages?

Is it common or recommended practice to have \n in panic messages?
Author
Collaborator

I don't see it as an explicitly recommended or discouraged practice, but since it is an error message I guess we can put it on the same line.

I don't see it as an explicitly recommended or discouraged practice, but since it is an error message I guess we can put it on the same line.
derppening force-pushed llvm-new-pass-manager from d5fb0ddfd9 to 769fd01df8 2023-09-18 11:35:39 +08:00 Compare
Author
Collaborator

v10: Changed panic error messages to be separated by : instead of \n

v10: Changed panic error messages to be separated by `:` instead of `\n`
sb10q merged commit 769fd01df8 into master 2023-09-18 11:36:43 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#319
No description provided.