Duplicate SpirvValueKind::ConstantPointer into a separate FnAddr variant. (#557)

crates/rustc_codegen_spirv/src/builder/builder_methods.rs

@@ -2075,33 +2075,59 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
 
     fn call(
         &mut self,
-        mut llfn: Self::Value,
+        callee: Self::Value,
         args: &[Self::Value],
         funclet: Option<&Self::Funclet>,
     ) -> Self::Value {
         if funclet.is_some() {
             self.fatal("TODO: Funclets are not supported");
         }
-        // dereference pointers
-        let (result_type, argument_types) = loop {
-            match self.lookup_type(llfn.ty) {
-                SpirvType::Pointer { .. } => {
-                    // Note that this doesn't necessarily mean a dynamic load, the function is
-                    // probably in cx.constant_pointers
-                    llfn = self.load(llfn, Align::from_bytes(0).unwrap());
-                }
+
+        // NOTE(eddyb) see the comment on `SpirvValueKind::FnAddr`, this should
+        // be fixed upstream, so we never see any "function pointer" values being
+        // created just to perform direct calls.
+        let (callee_val, result_type, argument_types) = match self.lookup_type(callee.ty) {
+            // HACK(eddyb) this seems to be needed, but it's not what `get_fn_addr`
+            // produces, are these coming from inside `rustc_codegen_spirv`?
             SpirvType::Function {
                 return_type,
                 arguments,
-                } => break (return_type, arguments),
-                ty => self.fatal(&format!("Calling non-function type: {:?}", ty)),
+            } => (callee.def(self), return_type, arguments),
+
+            SpirvType::Pointer { pointee } => match self.lookup_type(pointee) {
+                SpirvType::Function {
+                    return_type,
+                    arguments,
+                } => (
+                    match callee.kind {
+                        SpirvValueKind::FnAddr { function } => function,
+
+                        // Truly indirect call.
+                        _ => {
+                            let fn_ptr_val = callee.def(self);
+                            self.zombie(fn_ptr_val, "indirect calls are not supported in SPIR-V");
+                            fn_ptr_val
                         }
+                    },
+                    return_type,
+                    arguments,
+                ),
+                _ => bug!(
+                    "call expected `fn` pointer to point to function type, got `{}`",
+                    self.debug_type(pointee)
+                ),
+            },
+
+            _ => bug!(
+                "call expected function or `fn` pointer type, got `{}`",
+                self.debug_type(callee.ty)
+            ),
         };
+
         for (argument, argument_type) in args.iter().zip(argument_types) {
             assert_ty_eq!(self, argument.ty, argument_type);
         }
-        let llfn_def = llfn.def(self);
-        let libm_intrinsic = self.libm_intrinsics.borrow().get(&llfn_def).cloned();
+        let libm_intrinsic = self.libm_intrinsics.borrow().get(&callee_val).cloned();
         if let Some(libm_intrinsic) = libm_intrinsic {
             let result = self.call_libm_intrinsic(libm_intrinsic, result_type, args);
             if result_type != result.ty {
@@ -2114,7 +2140,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
             }
             result
         } else if [self.panic_fn_id.get(), self.panic_bounds_check_fn_id.get()]
-            .contains(&Some(llfn_def))
+            .contains(&Some(callee_val))
         {
             // HACK(eddyb) redirect builtin panic calls to an abort, to avoid
             // needing to materialize `&core::panic::Location` or `format_args!`.
@@ -2123,7 +2149,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
         } else {
             let args = args.iter().map(|arg| arg.def(self)).collect::<Vec<_>>();
             self.emit()
-                .function_call(result_type, None, llfn_def, args)
+                .function_call(result_type, None, callee_val, args)
                 .unwrap()
                 .with_type(result_type)
         }

crates/rustc_codegen_spirv/src/builder_spirv.rs

@@ -14,6 +14,14 @@ use std::{fs::File, io::Write, path::Path};
 pub enum SpirvValueKind {
     Def(Word),
 
+    // FIXME(eddyb) this shouldn't be needed, but `rustc_codegen_ssa` still relies
+    // on converting `Function`s to `Value`s even for direct calls, the `Builder`
+    // should just have direct and indirect `call` variants (or a `Callee` enum).
+    // FIXME(eddyb) document? not sure what to do with the `ConstantPointer` comment.
+    FnAddr {
+        function: Word,
+    },
+
     /// There are a fair number of places where `rustc_codegen_ssa` creates a pointer to something
     /// that cannot be pointed to in SPIR-V. For example, constant values are frequently emitted as
     /// a pointer to constant memory, and then dereferenced where they're used. Functions are the
@@ -69,7 +77,9 @@ impl SpirvValue {
                 Some(initializer.with_type(ty))
             }
 
-            SpirvValueKind::Def(_) | SpirvValueKind::LogicalPtrCast { .. } => None,
+            SpirvValueKind::FnAddr { .. }
+            | SpirvValueKind::Def(_)
+            | SpirvValueKind::LogicalPtrCast { .. } => None,
         }
     }
 
@@ -89,6 +99,21 @@ impl SpirvValue {
     pub fn def_with_span(self, cx: &CodegenCx<'_>, span: Span) -> Word {
         match self.kind {
             SpirvValueKind::Def(word) => word,
+            SpirvValueKind::FnAddr { .. } => {
+                if cx.is_system_crate() {
+                    *cx.zombie_undefs_for_system_fn_addrs
+                        .borrow()
+                        .get(&self.ty)
+                        .expect("FnAddr didn't go through proper undef registration")
+                } else {
+                    cx.tcx
+                        .sess
+                        .err("Cannot use this function pointer for anything other than calls");
+                    // Because we never get beyond compilation (into e.g. linking),
+                    // emitting an invalid ID reference here is OK.
+                    0
+                }
+            }
 
             SpirvValueKind::ConstantPointer {
                 initializer: _,

crates/rustc_codegen_spirv/src/codegen_cx/mod.rs

@@ -60,6 +60,7 @@ pub struct CodegenCx<'tcx> {
     /// Cache of all the builtin symbols we need
     pub sym: Rc<Symbols>,
     pub instruction_table: InstructionTable,
+    pub zombie_undefs_for_system_fn_addrs: RefCell<HashMap<Word, Word>>,
     pub libm_intrinsics: RefCell<HashMap<Word, super::builder::libm_intrinsics::LibmIntrinsic>>,
 
     /// Simple `panic!("...")` and builtin panics (from MIR `Assert`s) call `#[lang = "panic"]`.
@@ -120,6 +121,7 @@ impl<'tcx> CodegenCx<'tcx> {
             kernel_mode,
             sym,
             instruction_table: InstructionTable::new(),
+            zombie_undefs_for_system_fn_addrs: Default::default(),
             libm_intrinsics: Default::default(),
             panic_fn_id: Default::default(),
             panic_bounds_check_fn_id: Default::default(),
@@ -361,10 +363,36 @@ impl<'tcx> MiscMethods<'tcx> for CodegenCx<'tcx> {
         self.get_fn_ext(instance)
     }
 
+    // NOTE(eddyb) see the comment on `SpirvValueKind::FnAddr`, this should
+    // be fixed upstream, so we never see any "function pointer" values being
+    // created just to perform direct calls.
     fn get_fn_addr(&self, instance: Instance<'tcx>) -> Self::Value {
         let function = self.get_fn(instance);
         let span = self.tcx.def_span(instance.def_id());
-        self.make_constant_pointer(span, function)
+
+        let ty = SpirvType::Pointer {
+            pointee: function.ty,
+        }
+        .def(span, self);
+
+        if self.is_system_crate() {
+            // Create these undefs up front instead of on demand in SpirvValue::def because
+            // SpirvValue::def can't use cx.emit()
+            self.zombie_undefs_for_system_fn_addrs
+                .borrow_mut()
+                .entry(ty)
+                .or_insert_with(|| {
+                    // We want a unique ID for these undefs, so don't use the caching system.
+                    self.emit_global().undef(ty, None)
+                });
+        }
+
+        SpirvValue {
+            kind: SpirvValueKind::FnAddr {
+                function: function.def_cx(self),
+            },
+            ty,
+        }
     }
 
     fn eh_personality(&self) -> Self::Value {