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.

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

Singlepass aarch64 #2750

Merged
merged 37 commits into from Jan 27, 2022
Merged

Singlepass aarch64 #2750

merged 37 commits into from Jan 27, 2022

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Jan 12, 2022

Description

Add Aarch64 support on Singlapass, for both Linux and MacOS.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Jan 12, 2022

Not ready to merge. Mac support is not here yet.

@ptitSeb ptitSeb requested a review from Amanieu January 13, 2022 12:54
Comment on lines +3255 to +3250
.iter()
.cloned(),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it better to do into_iter() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can use into_iter() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'll still need a .cloned() so I think it will not change much in the end.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Jan 14, 2022

Ready to merge now

Copy link
Contributor

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

I can see a lot of unimplemented!() in machine_arm64.rs. Generally unimplemented! means the same thing as todo!: "this code is not implemented yet but needs to be implemented".

Use unreachable! instead to indicate "I am sure that this code is unreachable". They both panic, but it makes it clearer whether there is still work to be done.

NEON::V31,
];
match n {
0..=15 => Ok(REGS[n]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? Should this be 31? If not then add a comment explaining why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, typo yes.

Comment on lines 135 to 138
match n {
0..=31 => Ok(REGS[n]),
_ => Err(()),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match n {
0..=31 => Ok(REGS[n]),
_ => Err(()),
}
REGS.get(n).ok_or(())

get returns an Option, ok_or converts it to a Result.

But perhaps this function should be changed to use plain indexing and simply panic if an out-of-range value is passed in since you're always unwraping the result anyways.

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's not happy "expected enum arm64_decl::GPR, found &arm64_decl::GPR"

Copy link
Contributor

Choose a reason for hiding this comment

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

You can used .cloned() to turn an Option<&T> to an Option<T>.


type Assembler = VecAssembler<Aarch64Relocation>;

/// Force `dynasm!` to use the correct arch (x64) when cross-compiling.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Force `dynasm!` to use the correct arch (x64) when cross-compiling.
/// Force `dynasm!` to use the correct arch (aarch64) when cross-compiling.

Comment on lines 252 to 254
if (disp & 0x7) != 0 || (disp >= 0x8000) {
unreachable!();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this should use assert! instead.


fn emit_mov_imm(&mut self, dst: Location, val: u64) {
match dst {
Location::GPR(dst) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to encode the immediate as a logical immediate so it can be instantiated with ORR dst, XZR, #imm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. also can use MOVN if !val is small enough.
But all this are optimisation that can come later.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a todo comment with that suggestion so we log it somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The are a few relatively easy optimisation here and there, not just this one (like using LDP/STP more), maybe a notion page?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, let's create a notion doc!

),
}
}
fn emit_add2(&mut self, sz: Size, src: Location, dst: Location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be a wrapper over emit_add?

offset: reloc_at as u32,
addend: 0,
});
self.assembler.emit_movk(Location::GPR(GPR::X27), 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The first one should be movz.


pub struct MachineARM64 {
assembler: Assembler,
used_gprs: HashSet<GPR>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This should probably be a bitset instead of a hash table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the small size of the set? I have use HashSet to be consistant with the x64, that used HashSet too.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Jan 14, 2022

I can see a lot of unimplemented!() in machine_arm64.rs. Generally unimplemented! means the same thing as todo!: "this code is not implemented yet but needs to be implemented".

Use unreachable! instead to indicate "I am sure that this code is unreachable". They both panic, but it makes it clearer whether there is still work to be done.

I have use unimplemented!() in place where an implementation should exist. At least for the Atomic movement. And I used unreachable!() in place were I didn't expected an implementation to be needed. But I may have letfs some unimplemented!()in wrong places, but I did had the same schema in mind.

@Amanieu
Copy link
Contributor

Amanieu commented Jan 14, 2022

I see unimplemented for things like load_address, location_and, etc.

@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

@bors bors bot merged commit cfcc891 into master Jan 27, 2022
@bors bors bot deleted the singlepass_aarch64 branch January 27, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants