Wasmtime GitHub notifications bot (Aug 30 2022 at 21:23): Wasmtime GitHub notifications bot (Aug 30 2022 at 21:23):jameysharp commented on issue #4826:
Nice, that's surprisingly painless.
Is 16-byte alignment always best? Could we get the maximum alignment of anything in the vcode constant pool instead, and if so, is that extra complexity worth-while?
I suspect we don't care about wasting up to 15 extra bytes in the text section per function, and I know there are sometimes performance advantages to aligning x86 labels to 16-byte boundaries. So I imagine just picking a constant 16-byte alignment is not only good enough, but occasionally helpful. But I'm still curious if there are effects we care about.
Wasmtime GitHub notifications bot (Aug 30 2022 at 21:41):elliottt commented on issue #4826:
Nice, that's surprisingly painless.
Is 16-byte alignment always best? Could we get the maximum alignment of anything in the vcode constant pool instead, and if so, is that extra complexity worth-while?
I suspect we don't care about wasting up to 15 extra bytes in the text section per function, and I know there are sometimes performance advantages to aligning x86 labels to 16-byte boundaries. So I imagine just picking a constant 16-byte alignment is not only good enough, but occasionally helpful. But I'm still curious if there are effects we care about.
That's what I thought about doing initially, but @alexcrichton pointed out that llvm always aligns functions to 16-byte boundaries, so it seemed easier to do that and potentially waste some bytes.
Wasmtime GitHub notifications bot (Aug 30 2022 at 23:33):elliottt commented on issue #4826:
Before we merge this:
- @bjorn3 how do the changes to
cranelift-objectandcranelift-jitlook?- @uweigand is the default function alignment of 4 bytes for s390x correct?
Wasmtime GitHub notifications bot (Aug 31 2022 at 07:23):bjorn3 commented on issue #4826:
For the jit all references to EXECUTABLE_DATA_ALIGNMENT should use this new method. For object there is a reference at https://github.com/bytecodealliance/wasmtime/blob/186c7c3b89c0e172ff39c0126ca71bf9e995dc8c/cranelift/object/src/backend.rs#L351 and equivalent for
define_function. It needs anotherstd::cmp::minwrapper.
Wasmtime GitHub notifications bot (Aug 31 2022 at 09:16):uweigand commented on issue #4826:
Before we merge this:
* @uweigand is the default function alignment of 4 bytes for s390x correct?The minimum alignment required for correctness is just 2 bytes, and this should already come from
isa.symbol_alignment()as @bjorn3 points out. That said, having a default alignment of 4 bytes seems reasonable for performance reasons. (Maybe even higher, but we should do performance measurements first to make sure it's an overall win. So 4 is good for now.)
Wasmtime GitHub notifications bot (Aug 31 2022 at 16:33):elliottt commented on issue #4826:
For the jit all references to EXECUTABLE_DATA_ALIGNMENT should use this new method. For object there is a reference at
and equivalent for
define_function. It needs anotherstd::cmp::minwrapper.I removed all uses of
EXECUTABLE_DATA_ALIGNMENT, falling back on the max of the ISA's function alignment, symbol alignment, and the compiled function's required alignment. For follow-on work, would it be reasonable to remove thefunction_alignmentfield of theObjectBuilderincranelift-object?
Wasmtime GitHub notifications bot (Sep 01 2022 at 07:28):bjorn3 commented on issue #4826:
For follow-on work, would it be reasonable to remove the function_alignment field of the ObjectBuilder in cranelift-object?
I do think so. All it seems to be used for is lucet to set function alignment to 16 bytes, which is exactly what this PR did.
Last updated: Dec 23 2024 at 13:07 UTC