4 ms·

I'm not sure the example template was supposed to be canonical, vs demonstrating multiple output destructuring. Certainly the actual instruction definition in t

by tpmoney 2mo ago

I'm not sure the example template was supposed to be canonical, vs demonstrating multiple output destructuring. Certainly the actual instruction definition in the checker library seems to understand that there could be two possible inputs: https://github.com/odin-lang/Odin/blob/4247507dd5e31c9fd87166f53fd67b007ecca86c/core/rexcode/isa/x86/tablegen/instruction_table.odin https://github.com/odin-lang/Odin/blob/4247507dd5e31c9fd8716... But I'm also not entirely sure why the example should not have compiled. It seems to me that the idea here is to be able to define a typed set of something equivalent to a function that inlines some assembly, but nothing about that inherently requires that the number of input or output parameters to the template match the parameters in the underlying assembly calls. There's no reason (in my mind anyway) why this shouldn't be a perfectly valid template: // Returns the extended feature flags obtained by calling CPUID // with EAX=7 and ECX=1 cpu_extended_feature_flags :: asm() -> (a, b, c, d: u32) [ a = %eax, b = %ebx, c = %ecx, d = %edx, ] { mov %eax 0x7 mov %ecx 0x1 cpuid }

tialaramex 2mo ago
> But I'm also not entirely sure why the example should not have compiled The source you presented seems fine - it's explicitly setting the register. The trouble with the cpuid definition in the article is that it just doesn't set ECX at all It does actually seem as though Odin is intended to notice this problem but maybe is fooled that the register is pinned (because we want its result value) and so the diagnostic doesn't trigger. Reading this code reminded me that my annual summer leave ends this weekend because it would get so much review feedback if he worked with me. "Commenting out" blocks of code is NOT OK and neither are "if (false)" blocks.
- tpmoney 2mo ago
  Sure, I agree that if you're making a CPUID inline assembly template that isn't setting the register value, and it accepts any values for EAX that would also cause it to read ECX then the behavior is going to be undefined and likely unexpected, but that doesn't seem to be a reason for this not to compile. For one, requiring an input parameter when it isn't mandatory would mean that you're spending cycles setting a register with a value that you don't need and is just going to be overwritten anyway. A good number of the CPUID calls never read from ECX, and if you're going to call any of them, then the value in ECX is irrelevant. And sure, it's only an extra instruction or two, but presumably if you're dropping down to inline assembly, you kind of care about every wasted instruction. Again, this seems to me like it should be a perfectly valid assembly template (based on https://www.felixcloutier.com/x86/cpuid https://www.felixcloutier.com/x86/cpuid): // Returns the maximum input value for basic CPUID information cpu_extended_feature_flags :: asm() -> (a, b, c, d: u32) [ a = %eax, b = %ebx, c = %ecx, d = %edx, ] { mov %eax 0x0 cpuid } I admit I never touch inline assembly or really assembly at all save for the occasional microcontroller project, so maybe I'm missing something that's obvious to people more familiar with this. But to me the article seems to be sayin that the Odin templates will type check and validate that IF you have inputs from Odin types that are being put into registers or used as operands to your assembly, or are mapping registers and outputs from your assembly back to Odin types, that those mappings will be type compatible, and when you get those mappings wrong, you'll get a more useful error. But I didn't read it as saying that it will prevent you from writing assembly that does something completely unrelated to those inputs or outputs.
  - tialaramex 2mo ago
    For CPUID specifically this isn't going to be on a fast path. As its name might suggest it's a way to have the runtime CPU tell us what features are available so that we can adopt different implementations of perf-sensitive routines for that CPU. So e.g. maybe the CAD software executes a dozen CPUID instructions during startup, and then based on those it uses AVX512 vectorized operations later on some hardware but uses SSE instead on other hardware. I haven't read Intel's manual for a modern x86-64 CPU, and I certainly haven't read the community notes about this stuff which would tell you if, despite the documentation you need to behave differently, but my assumption would be that everybody clears ECX if they don't want a non-zero subleaf and that this is either known to be necessary or an obvious way to avoid nasty surprises in code that is never perf critical.
    - tpmoney 2mo ago
      I think we might be talking past each other here. Whether the assembly op is `cpuid` or something else, isn’t really my driving concern. The comment that started this thread (which I’m pretty sure wasn’t yours, so I don’t necessarily expect you to have an answer) was that they didn’t think this example should have compiled because `cpuid` may read from a second register as part of its operations, depending on the value passed to the first register. But that seems to be saying that the inline assembly template should have the same shape as the underlying assembly calls that are made. If the underlying call might read 3 registers, then the complaint appears to be that the templating system MUST require a template that calls that assembly to also have 3 inputs, even if it won’t use all 3. My thinking on this is the templating is a syntax for function declarations where the function is assembly code and not more higher level language code. We don’t require that functions have the same number of inputs and outputs that the code called within the function has in order for it to compile, I don’t see why it would be necessary for the assembly templates to work any differently. I get that `cpuid` specifically isn’t likely to run in the hot path of any code, but the underlying principle is the same. If you’re dropping to assembly, you likely have some performance tuning you’re trying to do. A templating system that requires your template inputs to 1:1 map to all the possible inputs of all the assembly calls you use in the template, regardless of wether you need or use them is adding extra instructions and waste for (to me) no obvious benefits.

by tpmoney 2mo ago

tialaramex 2mo ago

> But I'm also not entirely sure why the example should not have compiled The source you presented seems fine - it's explicitly setting the register. The trouble with the cpuid definition in the article is that it just doesn't set ECX at all It does actually seem as though Odin is intended to notice this problem but maybe is fooled that the register is pinned (because we want its result value) and so the diagnostic doesn't trigger. Reading this code reminded me that my annual summer leave ends this weekend because it would get so much review feedback if he worked with me. "Commenting out" blocks of code is NOT OK and neither are "if (false)" blocks.

tpmoney 2mo ago
Sure, I agree that if you're making a CPUID inline assembly template that isn't setting the register value, and it accepts any values for EAX that would also cause it to read ECX then the behavior is going to be undefined and likely unexpected, but that doesn't seem to be a reason for this not to compile. For one, requiring an input parameter when it isn't mandatory would mean that you're spending cycles setting a register with a value that you don't need and is just going to be overwritten anyway. A good number of the CPUID calls never read from ECX, and if you're going to call any of them, then the value in ECX is irrelevant. And sure, it's only an extra instruction or two, but presumably if you're dropping down to inline assembly, you kind of care about every wasted instruction. Again, this seems to me like it should be a perfectly valid assembly template (based on https://www.felixcloutier.com/x86/cpuid https://www.felixcloutier.com/x86/cpuid): // Returns the maximum input value for basic CPUID information cpu_extended_feature_flags :: asm() -> (a, b, c, d: u32) [ a = %eax, b = %ebx, c = %ecx, d = %edx, ] { mov %eax 0x0 cpuid } I admit I never touch inline assembly or really assembly at all save for the occasional microcontroller project, so maybe I'm missing something that's obvious to people more familiar with this. But to me the article seems to be sayin that the Odin templates will type check and validate that IF you have inputs from Odin types that are being put into registers or used as operands to your assembly, or are mapping registers and outputs from your assembly back to Odin types, that those mappings will be type compatible, and when you get those mappings wrong, you'll get a more useful error. But I didn't read it as saying that it will prevent you from writing assembly that does something completely unrelated to those inputs or outputs.
- tialaramex 2mo ago
  For CPUID specifically this isn't going to be on a fast path. As its name might suggest it's a way to have the runtime CPU tell us what features are available so that we can adopt different implementations of perf-sensitive routines for that CPU. So e.g. maybe the CAD software executes a dozen CPUID instructions during startup, and then based on those it uses AVX512 vectorized operations later on some hardware but uses SSE instead on other hardware. I haven't read Intel's manual for a modern x86-64 CPU, and I certainly haven't read the community notes about this stuff which would tell you if, despite the documentation you need to behave differently, but my assumption would be that everybody clears ECX if they don't want a non-zero subleaf and that this is either known to be necessary or an obvious way to avoid nasty surprises in code that is never perf critical.
  - tpmoney 2mo ago
    I think we might be talking past each other here. Whether the assembly op is `cpuid` or something else, isn’t really my driving concern. The comment that started this thread (which I’m pretty sure wasn’t yours, so I don’t necessarily expect you to have an answer) was that they didn’t think this example should have compiled because `cpuid` may read from a second register as part of its operations, depending on the value passed to the first register. But that seems to be saying that the inline assembly template should have the same shape as the underlying assembly calls that are made. If the underlying call might read 3 registers, then the complaint appears to be that the templating system MUST require a template that calls that assembly to also have 3 inputs, even if it won’t use all 3. My thinking on this is the templating is a syntax for function declarations where the function is assembly code and not more higher level language code. We don’t require that functions have the same number of inputs and outputs that the code called within the function has in order for it to compile, I don’t see why it would be necessary for the assembly templates to work any differently. I get that `cpuid` specifically isn’t likely to run in the hot path of any code, but the underlying principle is the same. If you’re dropping to assembly, you likely have some performance tuning you’re trying to do. A templating system that requires your template inputs to 1:1 map to all the possible inputs of all the assembly calls you use in the template, regardless of wether you need or use them is adding extra instructions and waste for (to me) no obvious benefits.