-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Sema: catch invalid asm input operands #16047
base: master
Are you sure you want to change the base?
Conversation
7c8dac9
to
56d0cf8
Compare
updated |
6e19351
to
e4b516a
Compare
tests pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a rebase but after that I think it can be merged. Thanks.
src/Sema.zig
Outdated
if (try sema.typeRequiresComptime(uncasted_arg_ty)) { | ||
return sema.fail( | ||
block, | ||
input_op_src, | ||
"type '{}' is comptime-only and cannot be used for an assembly input operand", | ||
.{uncasted_arg_ty.fmt(pt)}, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still a proposal so I assume it's not implemented yet so this can be changed when that proposal gets implemented? Well let's wait for an answer from @Snektron.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPIR-V is a typed language so regardless of the status of that proposal, We still need to be able to pass types through asm inputs/outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would I do this?
if (try sema.typeRequiresComptime(uncasted_arg_ty)) {
const target = pt.zcu.getTarget();
const arch = target.cpu.arch;
const is_spirv = arch.isSpirV();
if (uncasted_arg_ty.zigTypeTag(zcu) != .Type and !is_spirv) {
return sema.fail(
block,
input_op_src,
"type '{}' is comptime-only and cannot be used for an assembly input operand",
.{uncasted_arg_ty.fmt(pt)},
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/and
/or
/. Aside from that, looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not properly implemented in SPIR-V yet in any case, feel free to do whatever for now. We'll make the necessary changes when its required.
Sorry for the late reply 😅
I don't know if disallowing non-sized types is the best solution because for example RISC-V has a zero register that is hardwired to zero so
: [x0] "{x0}" (@as(u0, 0)),
in a way could be valid butu0
is non-sized so it won't work.At the same time though you can argue that anything that isn't sized can't actually be in a register.
Fixes #7843 (wasn't an issue with just stage1 btw, stage2 too segfaults without this patch)