Skip to content

Add a new way to hold structs in PolymorphicValue#791

Merged
zasdfgbnm merged 23 commits intomainfrom
polymorphic-struct
Aug 29, 2023
Merged

Add a new way to hold structs in PolymorphicValue#791
zasdfgbnm merged 23 commits intomainfrom
polymorphic-struct

Conversation

@zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Aug 26, 2023

See note [Struct Support in PolymorphicValue] for description, and the new test PolymorphicValueTest.Struct for examples.

Base automatically changed from legacy-struct to main August 26, 2023 16:18
#include <memory>
#include <string>

namespace nvfuser {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add note describing the big picture of nvFuser struct support.

return tv->fusion()->metadataOf(tv);
}

Val* IrBuilder::structExpr(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to header file, because it now needs a template parameter

@zasdfgbnm zasdfgbnm marked this pull request as ready for review August 28, 2023 23:31
@zasdfgbnm
Copy link
Collaborator Author

!build

@zasdfgbnm zasdfgbnm requested a review from naoyam August 29, 2023 01:24
@zasdfgbnm zasdfgbnm changed the title New struct Add a new way to hold structs in Aug 29, 2023
@zasdfgbnm zasdfgbnm changed the title Add a new way to hold structs in Add a new way to hold structs in PolymorphicValue Aug 29, 2023
Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason to name this file with .inl? Personally I haven't seen this suffix. When should we use this suffix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inl seems to be the standard suffix for the case where you feel a header is too long, and want to put part of it in a separate file:
https://www.oreilly.com/library/view/c-cookbook/0596007612/ch02s06.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example in the linked page makes sense. It separates declarations and implementations. That doesn't seem to be the case with Struct?

Copy link
Collaborator Author

@zasdfgbnm zasdfgbnm Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case of Struct, I am considering StructHandle as an interface (with PolymorphicValue) for struct implementation, but Struct and Accessor and NotImplementedStruct as implementation detail for struct. I agree that this separation of interface vs detail is not accurate, but still, I think:

  1. It's better to put them in a separate file.
  2. I can not think of a better name than .inl for that separate file.

I can name it to struct.h, but I have never seen one header includes another header in its end for pulling in more thing. But I have seen the name inl being used a few times. There is an example for using *-inl.h for implementations like inline function and numeric_limits. https://github.com/pytorch/pytorch/blob/main/c10/util/Half-inl.h

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the explanation.

@zasdfgbnm zasdfgbnm merged commit 2b8aef8 into main Aug 29, 2023
@zasdfgbnm zasdfgbnm deleted the polymorphic-struct branch August 29, 2023 16:01
jacobhinkle pushed a commit that referenced this pull request Aug 30, 2023
See note `[Struct Support in PolymorphicValue]` for description, and the
new test `PolymorphicValueTest.Struct` for examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants