Use LLVM New Pass Manager #319
No reviewers
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#319
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "llvm-new-pass-manager"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 ofTargetMachine
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 supportsnative
, i.e. optimizing for the host machine, similar to other C compilers.Some questions before this MR is ready:
nac3artiq
module, thecpu
argument oftarget.create_target_machine
is set to""
. Would it be better if we change this to the specific CPU names, e.g.cortex-a9
forIsa::CortexA9
,generic-rv32
forIsa::RiscV32*
etc.Depends on #318 to be merged first.
Do you understand why it was set to ""? What are the consequences of this proposed change?
Not really.
git blame
appears to show that this change was made by you, so maybe you would have an idea?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.326ed52324
to966ce7a659
v2: Rebased against master.
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.
966ce7a659
to6f9c464230
v3: Rebased against master, added target CPU name to
TargetMachine
creation, and updatedcheck_demos.sh
script to allow arguments to the compiler.The PR is ready for review.
WIP: Use LLVM New Pass Managerto Use LLVM New Pass ManagerUse LLVM New Pass Managerto WIP: Use LLVM New Pass ManagerOn 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.
6f9c464230
to667ebc699c
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.
WIP: Use LLVM New Pass Managerto Use LLVM New Pass Manager@ -66,1 +67,4 @@
pub opt_level: OptimizationLevel,
/// Whether to use LLVM's Legacy Pass Manager.
pub legacy_pm: bool,
Why would we?
Mostly to diagnose problems that occur only using the Legacy PM, e.g. #315.
@ -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
Which option? Also it's better to open issues than write fixme comments.
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.
667ebc699c
to96f940f16c
v5: Fixed incorrect CPU name for RISC-V.
v6: Removed FIXME
96f940f16c
to47c507453c
@ -663,0 +658,4 @@
.create_target_machine(self.llvm_options.opt_level)
.expect("couldn't create target machine");
if self.llvm_options.legacy_pm {
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.
47c507453c
to443f7d37e1
v7: Rebased against master, removed
legacy_pm
configuration@ -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());
Why not just move these println into the panic message?
@ -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!();
same here
@ -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());
and here
443f7d37e1
to50703e515d
50703e515d
tod5fb0ddfd9
v8, v9: Fixed compilation error with 3c2696c15c, moved message into
panic!
@ -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());
Is it common or recommended practice to have \n in panic messages?
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.
d5fb0ddfd9
to769fd01df8
v10: Changed panic error messages to be separated by
:
instead of\n