[WIP] Structured items #46

kvark · 2019-07-16T19:53:15Z

The PR contains core missing pieces of #5: rich instructions and top-level items, such as Module.

kvark · 2019-07-17T02:52:36Z

I think the biggest thing missing is basically a function(Context, DataInstruction) ->StructuredInstruction. It could be implemented as a few auto-generated methods:

impl Context {
  pub fn lift_type(&mut self, raw: &Instruction) -> TypeToken;
  pub fn lift_extension(&mut self, raw: &Instruction) -> Extension;
  pub fn lift_operand(&mut self, <something>) -> Operand;
  ..etc
}

kvark · 2019-07-17T17:52:00Z

Looking at this some more... I'm able to generate the lift() functions that process and match operands, aka:

    pub fn lift_memory_model(
        &mut self,
        raw: &mr::Instruction,
    ) -> Result<structs::MemoryModel, LiftError> {
        if raw.class.opcode as u32 != 14u32 {
            return Err(LiftError::OpCode);
        }
        let mut operands = raw.operands.iter();
        Ok(structs::MemoryModel {
            addressing_model: (match operands.next() {
                Some(&mr::Operand::AddressingModel(ref value)) => Some(value.clone()),
                Some(_) => Err(OperandError::Wrong)?,
                None => None,
            })
            .ok_or(OperandError::Missing)?,
            memory_model: (match operands.next() {
                Some(&mr::Operand::MemoryModel(ref value)) => Some(value.clone()),
                Some(_) => Err(OperandError::Wrong)?,
                None => None,
            })
            .ok_or(OperandError::Missing)?,
        })
    }

My problem is that we need to enforce the semantics that is in the spec but nowhere to be seen in the schema, e.g.:

Entry Point must be the Result of an OpFunction instruction.
Interface is a list of of global OpVariable instructions

In the schema they are just IdRef things:

        { "kind" : "IdRef",                              "name" : "'Entry Point'" },
        { "kind" : "LiteralString",                      "name" : "'Name'" },
        { "kind" : "IdRef",          "quantifier" : "*", "name" : "'Interface'" }

So there appears to be incomplete information in the schema to build the proper structured API. We have 2 ways to proceed from here:

just implement all/most instructions manually, use the exact typing corresponding to the spec
associate operand names with types in the code generator

@antiagainst do you have a feel on which approach would work best for us? ^

kvark · 2019-07-18T19:48:25Z

@antiagainst I'm going with approach (2) currently, i.e.:

        match operand.name.trim_matches('\'') {
            "Length" => quote! { ConstantToken },
            "Entry Point" => quote! { FunctionToken },
            "Interface" => quote! { VariableToken },
            _ => quote! { TypeToken },
        }

So far, it seems to me that naming is fairly consistent and repetitive, so adding such a match is still a big win over manually writing the code.

Another issue I'm facing is that content in SPIRV appears to not be sequential. An OpEntryPoint point to OpFunction, for example. This is annoying because we want to be populating our context graph with function/instruction/constant/etc tokens, and we can use one unless we processed it first. I'll try to see if we can process all the functions first, before going for the mode setting ops, and whether this is big issue within a function/block as well.

antiagainst · 2019-07-19T02:36:01Z

Yes the JSON grammar was not a full-fledged encoding of the syntax validation rules. (My bad initially defining it that way; it was more meaning to be supporting a data driven approach in SPIRV-Tools.) And you pointed out the most noticeable one: IdRef. I initially thought about adding more sub cases to it but never got to it.

Generally I'm in favor of more autogen. (You can know that which all the codegen stuff I've in this project; although codegen means another thing in compiler so we should probably rename it. ;-P) So matching names as a start sounds good to me. I think a more principled solution would be to enhance the SPIR-V JSON grammar to encode this info. (I'm actually needing other stuff that is not in the upstream; category is added by me for this repo specifically and so you see we are not using an upstream grammar here, which is bad because need to manually merge with each version update.)

antiagainst · 2019-07-19T02:43:29Z

There are a few places that SPIR-V has forward references. The sections before types and global values are all "metadata" instructions; meaning they attach additional information to some other instruction. So frequently they do forward referencing, esp. entry point, execution mode, debugging instruction, annotations. (We can also have OpTypeForwardPointer forward references a type...) Sounds good to me to parse the "main" instructions first (the types, global values, and functions) and then getting back to those instructions before. (We have the full MR (which should be named into as DR) representation here so it should be doable.) Also we should fold those metadata instructions into their main instruction if possible. That's why I have a decoration field for most of the instructions.

kvark · 2019-07-19T15:30:21Z

This is obviously incomplete, WIP, etc, but at least it's at the point where it compiles. I don't think having more stuff piled up on top would be a good way to merge later. @antiagainst - do you think you could review this and consider landing if it's done in the preferred way?

antiagainst

Awesome, overall LGTM! Thanks @kvark for handling this! :)

I'd suggest to keep CLs small and focused in the future so that it's easier and faster to write and review. Also the commits left in history would be standalone enough for easy reverting if something goes wrong. :)

antiagainst · 2019-07-20T00:04:09Z

codegen/sr.rs

-    } else {
-        let kind = Ident::new(kind, Span::call_site());
-        quote! { spirv::#kind }
+    match kind {


This is just refining existing implementation and not related to the new feature we are implementing here, right? In the future, it would be nice to have separate PRs.

antiagainst · 2019-07-20T00:10:35Z

codegen/sr.rs

+    }
+}
+
+pub fn gen_sr_structs_and_lifts(grammar: &structs::Grammar) -> (String, String) {


Can we have some comments regarding this function?

(I wasn't doing good on this previously given it's just me playing with this repo. Now it's intended to be developed by more people so some documentation could really save newcomers efforts in understanding existing codebase. :)

antiagainst · 2019-07-20T00:16:26Z

rspirv/Cargo.toml

@@ -20,6 +20,7 @@ travis-ci = { repository = "gfx-rs/rspirv" }
 num = "0.2"
 derive_more = "0.7"
 clippy = { version = "0.0", optional = true }
+fxhash = "0.2"


Nit: can you sort these dependencies alphabetically?

antiagainst · 2019-07-20T00:28:13Z

codegen/sr.rs

-                        self.types.push(t);
-                        TypeToken::new(self.types.len() - 1)
-                    }
+                pub fn #func_name #param_list -> Token<Type> {


Here too. It would be nice to have a separate CL for this change and the autogen touched files.

antiagainst · 2019-07-20T00:30:03Z

rspirv/sr/context.rs

+type FastHashMap<K, V> = HashMap<K, V, BuildHasherDefault<FxHasher>>;
+type Index = u32;
+
+/// A structure holding some kind of a structured SPIRV data that can be


Nit: SPIR-V is the formal spelling. Unless we are in code where we cannot use -, let's use the formal name when possible.

... some kind of SPIR-V constructs that can ...

antiagainst · 2019-07-20T00:38:15Z

rspirv/sr/instructions.rs

@@ -0,0 +1,7 @@
+use spirv;
+use super::{
+	context::Token,


antiagainst · 2019-07-20T00:40:42Z

rspirv/sr/items.rs

+}
+
+impl Module {
+    pub fn from_data(module: &mr::Module) -> Result<Self, ConvertionError> {


Nit: from_data_representation

antiagainst · 2019-07-20T00:41:19Z

rspirv/sr/mod.rs

+
+
+#[derive(Clone, Debug)]
+pub enum OperandError {


Can we have documentation for public APIs?

antiagainst · 2019-07-20T00:45:31Z

rspirv/sr/structs.rs

@@ -0,0 +1,7 @@
+use spirv;


structs is not a very descriptive name for this module. It's too general and it implies data representation (which is another representation). Can we have a better one? What about op?

antiagainst · 2019-07-20T00:47:31Z

rspirv/sr/autogen_instruction.rs

    ExtInst {
-        set: spirv::Word,
+        set: Token<Type>,


Shouldn't all these be Token<Instruction>?

kvark · 2019-07-25T19:28:16Z

@antiagainst This PR becomes increasingly difficult to advance without either of #49 or #55 to be merged.

antiagainst · 2019-08-15T04:02:45Z

Just wanted to double check here. So this PR is not expected to be reviewed but expected to be split into smaller ones right?

kvark · 2019-08-15T04:09:55Z

@antiagainst correct. It's technically in the "Draft" status, which isn't ready for review.
I think I'm getting close to catching up to this PR...

antiagainst · 2019-08-15T04:49:28Z

Cool, thanks!

Jasper-Bekkers · 2020-10-07T12:30:42Z

This PR has been stale for about a year, mind if we close it?

antiagainst force-pushed the master branch from 16ee565 to bc3f515 Compare July 17, 2019 16:09

kvark force-pushed the sr-items branch from 5fa3cc0 to d3f1d0b Compare July 17, 2019 17:47

kvark force-pushed the sr-items branch from d3f1d0b to bf57b8c Compare July 18, 2019 19:44

kvark force-pushed the sr-items branch from bf57b8c to 2b2305e Compare July 19, 2019 15:28

kvark changed the title ~~Structured items~~ Structured items - part 1 Jul 19, 2019

antiagainst requested changes Jul 20, 2019

View reviewed changes

This was referenced Jul 21, 2019

sr: Generic Storage and Token linked to SPIR-V #49

Closed

sr: refactor codegen with matches over if/else #50

Merged

kvark force-pushed the sr-items branch from 7e0f049 to 8301b22 Compare July 25, 2019 18:08

kvark added 7 commits July 25, 2019 14:14

sr: Module and items

d2d0706

Basic lift operations, separate type structs, factored Context

1b5f456

sr: Function type loading

36c9045

Add basic test-data and sr module loading test

c602f9e

sr: move instruction enums and structs codegen into one function

5c726b6

sr: parse block label

bd2bb6f

Separate Terminator from Branch, implement lifts for them

f24306b

kvark force-pushed the sr-items branch from 8301b22 to f24306b Compare July 25, 2019 18:32

kvark changed the title ~~Structured items - part 1~~ [WIP] Structured items Jul 25, 2019

kvark added 2 commits July 26, 2019 00:04

Special OpPhi handling as a prelude to BasicBlock

6541019

sr: Instruction enum lift

e54782c

This was referenced Jul 26, 2019

Move OpPhi into BasicBlock #58

Closed

Structured module items #63

Merged

kvark closed this Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Structured items #46

[WIP] Structured items #46

kvark commented Jul 16, 2019

kvark commented Jul 17, 2019

kvark commented Jul 17, 2019

kvark commented Jul 18, 2019

antiagainst commented Jul 19, 2019

antiagainst commented Jul 19, 2019

kvark commented Jul 19, 2019

antiagainst left a comment

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

antiagainst Jul 20, 2019

kvark commented Jul 25, 2019

antiagainst commented Aug 15, 2019

kvark commented Aug 15, 2019

antiagainst commented Aug 15, 2019

Jasper-Bekkers commented Oct 7, 2020

[WIP] Structured items #46

[WIP] Structured items #46

Conversation

kvark commented Jul 16, 2019

kvark commented Jul 17, 2019

kvark commented Jul 17, 2019

kvark commented Jul 18, 2019

antiagainst commented Jul 19, 2019

antiagainst commented Jul 19, 2019

kvark commented Jul 19, 2019

antiagainst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Jul 25, 2019

antiagainst commented Aug 15, 2019

kvark commented Aug 15, 2019

antiagainst commented Aug 15, 2019

Jasper-Bekkers commented Oct 7, 2020