Skip to content

Commit

Permalink
print: always print attributes inline and with per-attribute #[...]…
Browse files Browse the repository at this point in the history 
… syntax. (#35)

This moves us from my ad-hoc `#{A, B, C}` to a Rust-style `#[A] #[B]
#[C]` attribute syntax, and removes the idea of a "attribute set
shorthand" (`attrs123 = #{...}` followed by uses of `#attrs123`), which
was more annoying than useful (attributes can't refer to other
attributes so there was no "exponential deduplication" need for it).

With `#[...]` syntax, it also makes sense to never say e.g. `OpDecorate`
(as the operand itself will contain the word `Decoration`), or to use
`#[name = "..."]` Rust-style syntax for `OpName`.

(I didn't touch `OpMember{Name,Decorate}`, might be worth having a
SPIR-T `struct` type declaration syntax just to clean those up but it
feels even more ad-hoc than usual - maybe it might make more sense after
#36?)

Comparison (using Kajiya's `assets/shaders/raster_simple_ps.hlsl`,
compiled by DXC, like #33):

|[**Before**](https://htmlpreview.github.io/?https://gist.github.com/eddyb/124d807950059893678ad55f51ee5107/raw/0-before-spirt%252335-raster_simple_ps.hlsl.structured.spirt.html)<br><sub>(click
for complete pretty HTML
example)</sub>|[**After**](https://htmlpreview.github.io/?https://gist.github.com/eddyb/124d807950059893678ad55f51ee5107/raw/1-after-spirt%252335-raster_simple_ps.hlsl.structured.spirt.html)<br><sub>(click
for complete pretty HTML example)</sub>|
|-|-|

|![image](https://github.com/EmbarkStudios/spirt/assets/77424/82ee846a-0ed7-4f6b-be4f-7c6ccd1ba2fa)|![image](https://github.com/EmbarkStudios/spirt/assets/77424/f725b9bd-7715-4c96-8e4e-e573e41a8478)|
  • Loading branch information
@eddyb
eddyb authored Jun 13, 2023
2 parents a21e9f2 + 8be9735 commit 7be6951
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 125 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased] - ReleaseDate

### Changed 🛠
- [PR#35](https://github.com/EmbarkStudios/spirt/pull/35) abandoned the custom
`#{A, B, C}` "attribute set" style in favor of Rust-like `#[A]` `#[B]` `#[C]`
(and always printing them inline, without any `attrs123` shorthands)
- [PR#33](https://github.com/EmbarkStudios/spirt/pull/33) replaced the `spv.OpFoo<imms>(IDs)`
style of pretty-printing with `spv.OpFoo(A: imm, B: ID, C: imm, ...)` (unified parenthesized
list of operands, with deemphasized operand names in `foo:` "named arguments" style)
Expand Down
6 changes: 2 additions & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,8 @@ fn main() -> @location(0) i32 {
<!-- NOTE(eddyb) BEGIN/END below processed by .github/workflows/check-examples.sh -->
<!-- BEGIN tests/data/for-loop.wgsl.spvasm.structured.spirt -->
```cxx
#{
spv.OpDecorate(spv.Decoration.Flat),
spv.OpDecorate(spv.Decoration.Location(Location: 0)),
}
#[spv.Decoration.Flat]
#[spv.Decoration.Location(Location: 0)]
global_var0 in spv.StorageClass.Output: s32

func0() -> spv.OpTypeVoid {
Expand Down
180 changes: 59 additions & 121 deletions src/print/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,13 @@ struct AllCxInterned;
/// (as part of [`Node::AllCxInterned`]) and referenced multiple times.
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
enum CxInterned {
AttrSet(AttrSet),
Type(Type),
Const(Const),
}

impl CxInterned {
fn category(self) -> &'static str {
match self {
Self::AttrSet(_) => "attrs",
Self::Type(_) => "type",
Self::Const(_) => "const",
}
Expand Down Expand Up @@ -326,9 +324,6 @@ impl<'a> Plan<'a> {
}

match interned {
CxInterned::AttrSet(attrs) => {
self.visit_attr_set_def(&self.cx[attrs]);
}
CxInterned::Type(ty) => {
self.visit_type_def(&self.cx[ty]);
}
Expand Down Expand Up @@ -379,7 +374,7 @@ impl<'a> Plan<'a> {

impl<'a> Visitor<'a> for Plan<'a> {
fn visit_attr_set_use(&mut self, attrs: AttrSet) {
self.use_interned(CxInterned::AttrSet(attrs));
self.visit_attr_set_def(&self.cx[attrs]);
}
fn visit_type_use(&mut self, ty: Type) {
self.use_interned(CxInterned::Type(ty));
Expand Down Expand Up @@ -610,7 +605,6 @@ impl<'a> Printer<'a> {

#[derive(Default)]
struct AnonCounters {
attr_sets: usize,
types: usize,
consts: usize,

Expand Down Expand Up @@ -650,20 +644,6 @@ impl<'a> Printer<'a> {
Use::CxInterned(interned) => {
use_count == 1
|| match interned {
CxInterned::AttrSet(attrs) => {
let AttrSetDef { attrs } = &cx[attrs];
attrs.len() <= 1
|| attrs.iter().any(|attr| {
// HACK(eddyb) because of how these
// are printed as comments outside
// the `#{...}` syntax, they can't
// work unless they're printed inline.
matches!(
attr,
Attr::Diagnostics(_) | Attr::SpvDebugLine { .. }
)
})
}
CxInterned::Type(ty) => {
let ty_def = &cx[ty];

Expand Down Expand Up @@ -720,7 +700,6 @@ impl<'a> Printer<'a> {
} else {
let ac = &mut anon_counters;
let counter = match use_kind {
Use::CxInterned(CxInterned::AttrSet(_)) => &mut ac.attr_sets,
Use::CxInterned(CxInterned::Type(_)) => &mut ac.types,
Use::CxInterned(CxInterned::Const(_)) => &mut ac.consts,
Use::Node(Node::GlobalVar(_)) => &mut ac.global_vars,
Expand Down Expand Up @@ -1267,30 +1246,20 @@ impl Use {
// FIXME(eddyb) avoid having to clone `String`s here.
name.clone()
};
let (name, name_style) = match self {
Self::CxInterned(CxInterned::AttrSet(_)) => {
(format!("#{name}"), printer.attr_style())
}

let name = match self {
Self::AlignmentAnchorForControlRegion(_)
| Self::AlignmentAnchorForControlNode(_)
| Self::AlignmentAnchorForDataInst(_) => ("".to_string(), Default::default()),
| Self::AlignmentAnchorForDataInst(_) => "".to_string(),

_ => (name, Default::default()),
_ => name,
};
let name = pretty::Styles {
pretty::Styles {
anchor: Some(anchor),
anchor_is_def: is_def,
..name_style
}
.apply(name);
match self {
Self::CxInterned(CxInterned::AttrSet(_)) => {
// HACK(eddyb) separate `AttrSet` uses from their target.
pretty::Fragment::new([name, pretty::Node::ForceLineSeparation])
}
_ => name.into(),
..Default::default()
}
.apply(name)
.into()
}
UseStyle::Inline => match *self {
Self::CxInterned(interned) => interned
Expand Down Expand Up @@ -1328,12 +1297,6 @@ impl Print for Use {
}

// Interned/module-stored nodes dispatch through the `Use` impl above.
impl Print for AttrSet {
type Output = pretty::Fragment;
fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
Use::CxInterned(CxInterned::AttrSet(*self)).print(printer)
}
}
impl Print for Type {
type Output = pretty::Fragment;
fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
Expand Down Expand Up @@ -1745,74 +1708,39 @@ impl Print for CxInterned {
type Output = AttrsAndDef;
fn print(&self, printer: &Printer<'_>) -> AttrsAndDef {
match *self {
Self::AttrSet(attrs) => AttrsAndDef {
attrs: pretty::Fragment::default(),
def_without_name: printer.cx[attrs].print(printer),
},
Self::Type(ty) => printer.cx[ty].print(printer),
Self::Const(ct) => printer.cx[ct].print(printer),
}
}
}

impl Print for AttrSet {
type Output = pretty::Fragment;
fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
printer.cx[*self].print(printer)
}
}

impl Print for AttrSetDef {
type Output = pretty::Fragment;
fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
let Self { attrs } = self;

let mut comments = SmallVec::<[_; 1]>::new();
let mut non_comment_attrs = SmallVec::<[_; 4]>::new();
for attr in attrs {
let (attr_style, attr) = attr.print(printer);
match attr_style {
AttrStyle::Comment => comments.push(attr),
AttrStyle::NonComment => non_comment_attrs.push(attr),
}
}

let non_comment_attrs = if non_comment_attrs.is_empty() {
None
} else {
// FIXME(eddyb) remove this special-case by having some mode for
// "prefer multi-line but admit a single-element compact form"
// (a comma that's always `,\n`, effectively)
let per_attr_prefix = if non_comment_attrs.len() > 1 {
Some(pretty::Node::ForceLineSeparation.into())
} else {
None
};

// FIXME(eddyb) apply `attr_style` to more than just `#{` and `}`.
Some(pretty::join_comma_sep(
printer.attr_style().apply("#{"),
non_comment_attrs.into_iter().map(|attr| {
pretty::Fragment::new(per_attr_prefix.clone().into_iter().chain([attr]))
}),
printer.attr_style().apply("}"),
))
};

pretty::Fragment::new(
non_comment_attrs
.into_iter()
.chain(comments)
attrs
.iter()
.map(|attr| attr.print(printer))
.flat_map(|entry| [entry, pretty::Node::ForceLineSeparation.into()]),
)
}
}

pub enum AttrStyle {
Comment,
NonComment,
}

impl Print for Attr {
type Output = (AttrStyle, pretty::Fragment);
fn print(&self, printer: &Printer<'_>) -> (AttrStyle, pretty::Fragment) {
match self {
Attr::Diagnostics(diags) => (
AttrStyle::Comment,
pretty::Fragment::new(
type Output = pretty::Fragment;
fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
let non_comment_attr = match self {
Attr::Diagnostics(diags) => {
return pretty::Fragment::new(
diags
.0
.iter()
Expand Down Expand Up @@ -1887,8 +1815,8 @@ impl Print for Attr {
])
})
.intersperse(pretty::Node::ForceLineSeparation.into()),
),
),
);
}

Attr::QPtr(attr) => {
let (name, params_inputs) = match attr {
Expand Down Expand Up @@ -1927,23 +1855,34 @@ impl Print for Attr {
pretty::join_comma_sep("(", [usage.0.print(printer)], ")"),
),
};
(
AttrStyle::NonComment,
pretty::Fragment::new([
printer
.demote_style_for_namespace_prefix(printer.attr_style())
.apply("qptr.")
.into(),
printer.attr_style().apply(name).into(),
params_inputs,
]),
)
pretty::Fragment::new([
printer
.demote_style_for_namespace_prefix(printer.attr_style())
.apply("qptr.")
.into(),
printer.attr_style().apply(name).into(),
params_inputs,
])
}

Attr::SpvAnnotation(spv::Inst { opcode, imms }) => (
AttrStyle::NonComment,
printer.pretty_spv_inst(printer.attr_style(), *opcode, imms, [None]),
),
Attr::SpvAnnotation(spv::Inst { opcode, imms }) => {
let wk = &spv::spec::Spec::get().well_known;

// HACK(eddyb) `#[spv.OpDecorate(...)]` is redundant (with its operand).
if [wk.OpDecorate, wk.OpDecorateString, wk.OpExecutionMode].contains(opcode) {
printer.pretty_spv_operand_from_imms(imms.iter().copied())
} else if *opcode == wk.OpName {
// HACK(eddyb) unlike `OpDecorate`, we can't just omit `OpName`,
// but pretending it's a SPIR-T-specific `#[name = "..."]`
// attribute should be good enough for now.
pretty::Fragment::new([
printer.attr_style().apply("name = ").into(),
printer.pretty_spv_operand_from_imms(imms.iter().copied()),
])
} else {
printer.pretty_spv_inst(printer.attr_style(), *opcode, imms, [None])
}
}
&Attr::SpvDebugLine {
file_path,
line,
Expand All @@ -1964,16 +1903,15 @@ impl Print for Attr {
} else {
format!("// at {file_path:?}:{line}:{col}")
};
(
AttrStyle::Comment,
printer.comment_style().apply(comment).into(),
)
return printer.comment_style().apply(comment).into();
}
&Attr::SpvBitflagsOperand(imm) => (
AttrStyle::NonComment,
printer.pretty_spv_operand_from_imms([imm]),
),
}
&Attr::SpvBitflagsOperand(imm) => printer.pretty_spv_operand_from_imms([imm]),
};
pretty::Fragment::new([
printer.attr_style().apply("#[").into(),
non_comment_attr,
printer.attr_style().apply("]").into(),
])
}
}

Expand Down

0 comments on commit 7be6951

Please sign in to comment.