codegen_llvm/llvm_type: avoid matching on the Rust type

[RalfJung]

This match is highly suspicious. Looking at scalar_llvm_type_at I think it makes no difference. But if it were to make a difference that would be a huge problem, since it doesn't look through repr(transparent)!

Cc @eddyb @bjorn3

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[davidtwco]

This looks okay to me but I want a second pair of eyes on it.

[davidtwco]

r? @bjorn3

[RalfJung]

I just found this which seems equally suspicious:

rust/compiler/rustc_codegen_llvm/src/type_of.rs

Lines 306 to 308 in 4354192

	// HACK(eddyb) special-case fat pointers until LLVM removes
	// pointee types, to avoid bitcasting every `OperandRef::deref`.
	match *self.ty.kind() {

Moreover, pointee types are removed, so the entire hack is probably unnecessary?

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

[RalfJung]

I've removed that other match as well now, and since the GCC backend copy-pasted that code I also updated it there.

Cranelift is extremely reliant on Rust types in its backend type computation functions, which is concerning... maybe it's just somehow not relevant for ABI there?

[bjorn3]

cg_clif should not be dependent on rust types for calling convention handling. It should only use rust types for handling locals.

[RalfJung]

Okay, I was probably looking at the wrong code then.

[bjorn3]

@bors r+

[bors]

📌 Commit 9b9cb51 has been approved by bjorn3

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9b9cb51 with merge 4e78abb...

[eddyb]

Sorry for seeing this so late, but, if not made clear so far by anyone else:
the special cases you removed are simultaneously sound and obsolete.

They're sound because they only affect pointer types' pointee types (which doesn't impact ABI on it own), and they're obsolete thanks to LLVM opaque pointers (making the removal welcome!).

You can kinda tell they were working fine because e.g. Option<&String> would look like i64*, whereas the inner &String would actually be {i8*, i64, i64}* - worst case, it would pessimize some badly written LLVM optimizations, but all of those concerns should be gone now.

In fact, some of this code can very likely be simplified!

scalar_pair_element_llvm_type only exists in the form it does because it needed to special-case wide pointers, but now each component of the scalar pair can be independently converted to LLVM.

It might even make sense to remove caches like scalar_lltypes and instead have something much simpler keyed by Primitive - the existing scalar_llvm_type_at might be good enough, even, and I would argue the cache only existed because of the pointer cases, and can now be fully removed.

[RalfJung]

the special cases you removed are simultaneously sound and obsolete.

Yes that was my conclusion, too. That still makes this PR a good cleanup, no? :)

But looking at these special cases led me to ones that aren't sound (see the recent ABI issues I opened), so the cleanup of obsolete-but-sound type matching was still worth it.

I'm sure there's further simplification possible.