better const folding for int and bool

[Firestar99]

see #300

const folding used to only support:

• basic int: add, mul
• casting from Optimize constant casts to avoid unnecessary capabilities #302

This PR adds const folding for:

• basic int: sub, div, rem, shr, lshl, ashl, neg, not (+ unchecked variants)
• basic bool: and, or, xor, not
• comparison for int and bool (icmp)
• select with const condition

Should probably add some diff tests

[Firestar99]

@eddyb Do you have any explanation why 43d5107 is required to make libm compile? This PR also adds explicit type checking for each operation, and for some reason libm calls add that meant for integers with two floats instead of calling fadd.

[eddyb]

Love to see this, and I would love to debug & land this ASAP (at a glance it seems reasonably done), can you rebase it?

[Firestar99]

There's nothing per se preventing this merge, I was going to wait on #321 for some free testing :D

But a quick local rebase on it and it all seems to pass, so feel free to merge.

[eddyb]

But a quick local rebase on it and it all seems to pass

Looks like the TypedBuffer (abi.rs fix + tests) changes are gone, which is what I was hoping for, thanks!

Also, what I thought was a blocker was #317 (comment) - but I see now that you're asking why you had to do such a hack (I would still want to investigate it, but we could open an issue and land this PR as-is).

[eddyb]

These comments are kind of funny - I forget exactly how this works but I believe e.g. unchecked_uadd(a, b) is uN::checked_add(a, b).unchecked_unwrap().

That is, overflowing becomes Undefined Behavior.

LLVM has flags that start with n for this (e.g. nuw/nsw meaning "no (un)signed wrap"),
and it turns out rustc_codegen_llvm is emitting exactly those flags.

SPIR-V seems to have added nuw/nsw equivalents in this extension, and later SPIR-V 1.4:
https://github.khronos.org/SPIRV-Registry/extensions/KHR/SPV_KHR_no_integer_wrap_decoration.html#_decorations

I don't expect you to implement using it (which would have to be conditional on the SPIR-V version or that extension being in target-features, anyway), but it would be nice to remove these (wrong-ish) comments, maybe replace them with a big // TODO or // FIXME before the block of unchecked_* etc.

[Firestar99]

sounds like something that should be an issue and handled in a followup

[eddyb]

I only brought it up because IIRC I saw // comments turn into /// comments.
If you keep them // comments I can kinda see the argument for it being preexisting, but it's still sad that it's effectively a lie, and we could at least turn them into e.g.:

// FIXME: take advantage of `unchecked_*` making overflow UB.

(feel free to workshop it further, but even that should be good enough to replace the incorrect comments with)

[eddyb]

Hmm, I don't see a failure path here (i.e. "what if const-folding fails"), does const_uint_big truncate? (truncation might be enough for both signed and unsigned integers)

[Firestar99]

Currently, none of the const-fold ops can fail.

does const_uint_big truncate?

It seems like it does

rust-gpu/crates/rustc_codegen_spirv/src/builder_spirv.rs

Lines 562 to 574 in 05b3449

	// HACK(eddyb) this is done so late (just before interning `val`) to
	// minimize any potential misuse from direct `def_constant` calls.
	let val = match (val, scalar_ty) {
	(SpirvConst::Scalar(val), Some(SpirvType::Integer(bits, signed))) => {
	let size = Size::from_bits(bits);
	SpirvConst::Scalar(if signed {
	size.sign_extend(val) as u128
	} else {
	size.truncate(val)
	})
	}
	_ => val,
	};

[eddyb]

Alright, this should be fine for both signed and unsigned, for both arithmetic and bitwise ops (tho the only bitwise op that would ever be a concern is flipping all the bits).

You only have to worry about anything that explicitly cares about the number of bits - e.g. instead of x.wrapping_shl(y) you should do x.checked_shl(y & ((x_size.bits() - 1) as u128)).unwrap(), and same for wrapping_shr.

If you add a test for something like 1u32 << 32, the result should be 1 (as the shift amount gets masked out and becomes 1u32 << 0) not 0 (like (1u128 << 32) as u32 would cause).

[eddyb]

Uhh why does wrapping_div even exist, it's not an operation that can overflow??

Okay, yeah, the docs do point that out:

Wrapped division on unsigned types is just normal division. There’s no way wrapping could ever happen. This function exists so that all operations are accounted for in the wrapping operations.

What you want is checked_div and to bail out of if it returns None. Which probably means that you want to replace if let Some(const_x) = f(x) {...} with f(x).and_then(|const_x| Some({...})), so that the expression can use ? (or even return None;, tho that would be confusing).

[Firestar99]

From the spirv spec:

OpUDiv: Behavior is undefined if Operand 2 is 0.
OpSDiv: Behavior is undefined if Operand 2 is 0. Behavior is undefined if Operand 2 is -1 and Operand 1 is the minimum representable value for the operands' type, causing signed overflow.

From Rust docs:

U: This operation will panic if other == 0.
S: This operation will panic if other == 0 or the division results in overflow.

(Does this panic require debug asserts on?)

If behavior in these rare cases is undefined in spirv anyway, why explicitly not const fold them for them to trigger UB during execution?

[eddyb]

(Does this panic require debug asserts on?)

No, it's always there, because there is no possible value it could give (unlike wrapping arithmetic).

If behavior in these rare cases is undefined in spirv anyway, why explicitly not const fold them for them to trigger UB during execution?

Maybe try adding a test? This code will panic and you will get an ICE, most likely.
At runtime, at least in safe Rust code, these divisions are gated behind a panic.

So e.g. this code:

let y = x / 0;

will generate MIR that looks like:

if 0 == 0 { panic!("division by zero"); }
let y = unsafe { x.unchecked_div(0) };

which should optimize to an unconditional panic (the actual UB after the panic never being reachable), not crash the compiler.

[eddyb]

Oh, that's funny, that's basically the exact situation unchecked_* is in, but only this has an accurate comment that mentions UB.

[eddyb]

It should be possible to show in a test that these misbehave - see #317 (comment)