Skip to content

@mulCarryless() - add a builtin to llvm backend #13483

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

Closed
wants to merge 1 commit into from

Conversation

travisstaloch
Copy link
Contributor

addresses #9631.

only works with llvm backend/x86 so far. allows new
test/behavior/mul_carryless.zig to pass with -Denable-llvm. doesn't do
any backend/arch/cpu-feature testing yet.

@travisstaloch
Copy link
Contributor Author

WIP - i need guidance on how to implement support for other backends and
and arch/cpu feature sets. specifically where/how should i check for
backend == llvm, arch == x86 etc?

@jedisct1
Copy link
Contributor

jedisct1 commented Nov 8, 2022

Having such builtin would be useful!

For clarity, I'd suggest providing the indices as two integers instead of a single one.

Intel opcodes are not the best model when it comes to clarity, and on other architectures, that index doesn't even exist (there are distinct instructions for hi/lo instead).

@travisstaloch
Copy link
Contributor Author

travisstaloch commented Nov 9, 2022

For clarity, I'd suggest providing the indices as two integers instead of a single one.

👍 Thanks for the feedback. Sounds like changing from imm: u8 to indices: [2]u8?

I will work on implementing this locally but wait a bit for more feedback before pushing a commit to avoid spamming ci. I'm still hoping for some guidance on how to do the arch/cpu feature checks and delegate to compiler_rt when necessary.

one thing i'm not sure about - does imm really have to be comptime known? from experience its better to avoid requiring comptime unless necessary. maybe i'll try to remove the comptime check from Sema and test this locally.

@travisstaloch
Copy link
Contributor Author

travisstaloch commented Nov 9, 2022

one thing i'm not sure about - does imm really have to be comptime known?

i got my answer. changing imm from const to var produces the following. so it is required to be comptime known.

$ zig-out/bin/zig test test/behavior/mul_carryless.zig
LLVM Emit Object... 
immarg operand has non-immediate parameter
  %15 = load i8, ptr %1, align 1, !dbg !4485
  %16 = call <2 x i64> @llvm.x86.pclmulqdq(<2 x i64> %13, <2 x i64> %14, i8 %15), !dbg !4485

thread 320228 panic: LLVM module verification failed
codegen/llvm.zig:775:17: 0xda18e0 in flushModule (zig)
link/Elf.zig:960:47: 0xdc126c in flushModule (zig)
link/Elf.zig:1250:29: 0xdb5e5b in linkWithLLD (zig)
link/Elf.zig:946:32: 0xbfb688 in flush (zig)

@jedisct1
Copy link
Contributor

jedisct1 commented Nov 9, 2022

Yes, the Intel opcode requires an immediate for the selector. And on RISCV and aarch64, this has to be translated to different instructions (and the same word is selected from both operands, so you need to shift if necessary).

Sounds like changing from imm: u8 to indices: [2]u8

How about two parameters: @clmul(x, y, sel_x, sel_y)?

addresses ziglang#9631. only works with llvm backend/x86 so far. allows new
test/behavior/mul_carryless.zig to pass with -Denable-llvm. doesn't do
any backend/arch/cpu-feature testing.
@andrewrk
Copy link
Member

Closing - no updates for 30 days and CI checks are failing. Please open a new PR if you want to continue these efforts.

@andrewrk andrewrk closed this Dec 12, 2022
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.

3 participants