[mlir][EmitC]Add a Reflection Map to a Class #150572

Jaddyen · 2025-07-25T04:51:58Z

This adds a pass that adds a getBufferForName function to EmitC classes that enables runtime lookup of field buffers by their string names.
This allows us to get the cpp emission:

#include <map>
#include <string>
class mainClass {
 public:
  float[1] fieldName0;
  float[1] fieldName1;
  float[1] fieldName2;
  
  const std::map<std::string, char*> v2 = {{"another_feature", reinterpret_cast<char*>(&fieldName0)}, { "some_feature", reinterpret_cast<char*>(&fieldName1)}, { "output_0", reinterpret_cast<char*>(&fieldName2)}};

  void execute() {
    size_t v1 = 0;
    float[1] v2 = fieldName0;
    float[1] v3 = fieldName1;
    float[1] v4 = fieldName2;
    float v5 = v3[v1];
    float v6 = v2[v1];
    float v7 = v5 + v6;
    v4[v1] = v7;
    return;
  }

};

jpienaar · 2025-07-25T07:12:50Z

Shouldn't it be "char* getBufferForName" ? (it returns a string)

mtrofin · 2025-07-25T14:10:19Z

The map should be declared as a member of the class.

ilovepi · 2025-07-25T16:01:02Z

mlir/include/mlir/Dialect/EmitC/Transforms/Passes.td

Suggested change

This would require that the class has fields with attributes and a function named `execute`.

This requires that the class has fields with attributes and a function named `execute`.

Why is execute a requirement?

I insert the new function before the execute function.
Ideally, we could have some other insertion point but execute func made the most sense since it is always added when we wrap-emitc-func-in-class.

ilovepi · 2025-07-25T16:12:39Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Since this code is repeated, I'd suggest making it a helper function or a lambda. Then the code here can be

if(!hasMap) addHeader(kMapLibraryHeader); if(!hasString) addHeader(kMapLibraryHeader);

ilovepi · 2025-07-25T16:13:39Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Suggested change

bool hasMap = false;

bool hasString = false;

bool hasMapHdr = false;

bool hasStringHdr = false;

nit: hasString is pretty common name that usually isn't about the header. Lets just make it completely unambiguous.

ilovepi · 2025-07-25T16:16:43Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

If you're going to early exit, you might as well put it in the loop.

ilovepi · 2025-07-25T16:30:00Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

should this be a cast<>? If you know it will be of this type (e.g. can't fail) then cast<> is appropriate. Otherwise, I think you still need to check dyn_cast<> for success.

ack, thanks for the pointer!

ilovepi · 2025-07-25T16:30:11Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Suggested change

std::string indexPath = stringAttr.getValue().str();

fieldNames.emplace_back(indexPath, fieldOp.getName().str());

fieldNames.emplace_back(tringAttr.getValue().str(), fieldOp.getName().str());

You can avoid a copy here, otherwise you can probably use std::move() to similar effect.

ilovepi · 2025-07-25T16:32:47Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Instead of nesting so deeply w/ dyn_cast<>, can you just early exit if the cast fails?

ilovepi · 2025-07-25T16:36:05Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

I'd recommend doing this w/ a string_stream. You can avoid a lot of copies and you can use things like formatv() to make the code easier to read.

ilovepi · 2025-07-25T16:40:21Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

For blocks like this, where you're basically spelling out the C++ code, you may want to write that down in a comment, so its easy to see what operations you're doing.

Since we now simply return the full map, I have reduced the amount of c++ code I'm spelling out.
I appreciate the pointer!

ilovepi · 2025-07-25T16:48:48Z

mlir/test/Dialect/EmitC/add_reflection_map.mlir

Should the \22 be in the output? I'd expect maybe \", but IDK if that's going to work correctly. It desn't seem right to me at least...

I think it is some weird parsing going on.
I chose to not take it too seriously since it didn't change the output when it came to cpp.

Jaddyen · 2025-07-29T04:15:29Z

The map should be declared as a member of the class.

We could:

Declare the map as a member of the class, pass it as an argument to the function then return it from the function or
Initialize the map within the function and return it from the function.

mtrofin · 2025-07-29T13:55:59Z

The map should be declared as a member of the class.

We could:

Declare the map as a member of the class, pass it as an argument to the function then return it from the function or

Initialize the map within the function and return it from the function.

Let's do 1. 2 would mean re-creating the map at every call to that function, which would need to be renamed to indicate that (create not get), but more importantly, it's not necessary to re-create the map as the data inside of it doesn't change and shouldn't change.

ilovepi · 2025-07-29T16:40:02Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Should we stop in the error case? I'd assume you'd want to return an error code here, instead of continuing.

ilovepi · 2025-07-29T16:48:54Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

Do you think this would be more readable as a formatv(). IMO, structures like this are harder to read when constructed via stream (e.g. its easier to misread them or miss a detail). WDYT?

ilovepi · 2025-07-29T16:51:14Z

mlir/lib/Dialect/EmitC/Transforms/AddReflectionMap.cpp

LLVM style is to omit braces for single statement bodies. I'm not sure if MLIR deviates (I'm fine either way), but we should be consistent, and you have a different convention a few lines above.

github-actions · 2025-07-29T21:10:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

This reverts commit f2dee0d99bc1fb258b6cef57dc150cb637cc4ab3.

…f-map

aniragil · 2025-08-03T08:13:55Z

[Apologies if I missed some earlier community discussion on this]
This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core?
If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.

+@marbre

marbre · 2025-08-04T11:44:54Z

[Apologies if I missed some earlier community discussion on this] This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core? If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.

+@marbre

Thanks @aniragil!

While there has been some discussion dating back to 2023 on what MLGO would need and resulting in the efforts by @simon-camp to add an upstream supported lowering to EmitC (PR #11754), it isn't clear to me what else is needed. Therefore, I would appreciate to discuss this based on an RFC as suggested by @aniragil.

mtrofin · 2025-08-04T14:35:01Z

[Apologies if I missed some earlier community discussion on this] This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core? If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.
+@marbre

Thanks @aniragil!

While there has been some discussion dating back to 2023 on what MLGO would need and resulting in the efforts by @simon-camp to add an upstream supported lowering to EmitC (PR #11754), it isn't clear to me what else is needed. Therefore, I would appreciate to discuss this based on an RFC as suggested by @aniragil.

The RFC in question is this one. The MLGO usecase was used as one of the motivations, especially since MLGO is in-tree. The additional requirements (for MLGO) were listed high-level, this patch here is for the "ability to bind by name" part.

Perhaps we should make that relation to the RFC more clear in this patch description?

marbre · 2025-08-04T15:10:08Z

[Apologies if I missed some earlier community discussion on this] This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core? If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.
+@marbre

Thanks @aniragil!
While there has been some discussion dating back to 2023 on what MLGO would need and resulting in the efforts by @simon-camp to add an upstream supported lowering to EmitC (PR #11754), it isn't clear to me what else is needed. Therefore, I would appreciate to discuss this based on an RFC as suggested by @aniragil.

The RFC in question is this one. The MLGO usecase was used as one of the motivations, especially since MLGO is in-tree. The additional requirements (for MLGO) were listed high-level, this patch here is for the "ability to bind by name" part.

Perhaps we should make that relation to the RFC more clear in this patch description?

That RFC was specifically about upstreaming the TOSA to EmitC conversions and the reference implementation, both implemented in https://github.com/iml130/mlir-emitc/. It is correct that MLGO use-case was highlighted as a motivation but the specific RFC never got a lot of attraction and was never accepted. I think what Gil is asking for is a separate, more detailed RFC with regards to what is needed and what operations or conversions need to be implemented. It can of course refer to the linked thread and re-use arguments.

mtrofin · 2025-08-04T15:40:54Z

[Apologies if I missed some earlier community discussion on this] This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core? If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.
+@marbre

Thanks @aniragil!
While there has been some discussion dating back to 2023 on what MLGO would need and resulting in the efforts by @simon-camp to add an upstream supported lowering to EmitC (PR #11754), it isn't clear to me what else is needed. Therefore, I would appreciate to discuss this based on an RFC as suggested by @aniragil.

The RFC in question is this one. The MLGO usecase was used as one of the motivations, especially since MLGO is in-tree. The additional requirements (for MLGO) were listed high-level, this patch here is for the "ability to bind by name" part.
Perhaps we should make that relation to the RFC more clear in this patch description?

That RFC was specifically about upstreaming the TOSA to EmitC conversions and the reference implementation, both implemented in https://github.com/iml130/mlir-emitc/. It is correct that MLGO use-case was highlighted as a motivation but the specific RFC never got a lot of attraction and was never accepted.

Right, and IIRC there was no explicit RFC signoff process at the time anyway; on that - asking to learn (and make sure we follow the right steps) - is there an explicit signoff now in MLIR, or, like in LLVM, that's ony an escalation when there's disagreements?

I think what Gil is asking for is a separate, more detailed RFC with regards to what is needed and what operations or conversions need to be implemented. It can of course refer to the linked thread and re-use arguments.

We should have one up today. I am concerned with timing here, though, and would love it if we could find a way to make progress in the meantime. I'm assuming there's no objection to other work continuing, like lowering opcodes (that's quite generic), while folks look at the pieces that are more MLGO-specific and covered by the RFC, correct?

simon-camp · 2025-08-04T16:08:30Z

As this pass is very use case specific, it could also be moved to the MLGO side of the repo together with a custom opt tool to run it.

Jaddyen · 2025-08-05T22:33:06Z

[Apologies if I missed some earlier community discussion on this] This patch seems to be one in a series aimed at supporting specific MLGO features. Would be good if we could separate generic contributions that benefit most/all EmitC users (e.g. adding an emit.class op) from downstream-specific ones. For instance, the pass added here seems to perform a rather specific transformation and rely on existing dialect components. Could you elaborate on why it belongs upstream in MLIR core? If you believe these patterns (reflection map, func-to-class for AoT) to be beneficial for many EmitC users, would be great if you could post an RFC on the MLIR Discourse to facilitate a wider discussion in the community.
+@marbre

Thanks @aniragil!
While there has been some discussion dating back to 2023 on what MLGO would need and resulting in the efforts by @simon-camp to add an upstream supported lowering to EmitC (PR #11754), it isn't clear to me what else is needed. Therefore, I would appreciate to discuss this based on an RFC as suggested by @aniragil.

The RFC in question is this one. The MLGO usecase was used as one of the motivations, especially since MLGO is in-tree. The additional requirements (for MLGO) were listed high-level, this patch here is for the "ability to bind by name" part.
Perhaps we should make that relation to the RFC more clear in this patch description?

That RFC was specifically about upstreaming the TOSA to EmitC conversions and the reference implementation, both implemented in https://github.com/iml130/mlir-emitc/. It is correct that MLGO use-case was highlighted as a motivation but the specific RFC never got a lot of attraction and was never accepted. I think what Gil is asking for is a separate, more detailed RFC with regards to what is needed and what operations or conversions need to be implemented. It can of course refer to the linked thread and re-use arguments.

Here is an RFC.

aniragil · 2025-08-06T11:29:00Z

I am concerned with timing here, though, and would love it if we could find a way to make progress in the meantime.

One way to try and speed things up is to request more EmitC folks in the community for review in addition to @marbre and myself, e.g. @simon-camp, @mgehre-amd, @jacquesguan.

I'm assuming there's no objection to other work continuing, like lowering opcodes (that's quite generic), while folks look at the pieces that are more MLGO-specific and covered by the RFC, correct?

It's eventually up to the reviewers, so best to upload and have concrete discussions per case. For instance, malloc and memcpy were such generic contributions.

Jaddyen requested review from ilovepi, jpienaar and mtrofin July 25, 2025 06:04

ilovepi reviewed Jul 25, 2025

View reviewed changes

ajaden-codes force-pushed the add-ref-map branch 2 times, most recently from f4512f1 to 94b0c34 Compare July 28, 2025 16:13

Jaddyen marked this pull request as ready for review July 29, 2025 04:15

ilovepi reviewed Jul 29, 2025

View reviewed changes

Jaddyen requested a review from ilovepi July 29, 2025 21:11

Jaddyen marked this pull request as draft July 29, 2025 21:27

Jaddyen added 13 commits August 1, 2025 18:27

Modeling

58012dd

Add an argument

da210bd

Specify the pass reqs

afe86ff

small change

e3c958a

avoid re-initialization

81ac87e

Revert "avoid re-initialization"

f1cb9df

This reverts commit f2dee0d99bc1fb258b6cef57dc150cb637cc4ab3.

Cleaning

a165ba4

Return the whole map

b8ecc0c

Format the string well

4595328

working test

e1402cf

make it a member

d30e6b4

use ternary

a443c84

specify the reflection map name

8674e74

Jaddyen added 2 commits August 1, 2025 20:00

update test

ac8e562

Merge remote-tracking branch 'refs/remotes/upstream/main' into add-re…

cde2287

…f-map

Jaddyen force-pushed the add-ref-map branch from 750ba11 to cde2287 Compare August 1, 2025 21:53

Jaddyen marked this pull request as ready for review August 1, 2025 21:54

Jaddyen added 2 commits August 1, 2025 22:29

update td file

1a4b8b9

remove wrong field

17c6318

	This would require that the class has fields with attributes and a function named `execute`.
	This requires that the class has fields with attributes and a function named `execute`.

-    bool hasMap = false;
-    bool hasString = false;
+    bool hasMapHdr = false;
+    bool hasStringHdr = false;

	std::string indexPath = stringAttr.getValue().str();
	fieldNames.emplace_back(indexPath, fieldOp.getName().str());
	fieldNames.emplace_back(tringAttr.getValue().str(), fieldOp.getName().str());

[mlir][EmitC]Add a Reflection Map to a Class #150572

Are you sure you want to change the base?

[mlir][EmitC]Add a Reflection Map to a Class #150572

Uh oh!

Conversation

Jaddyen commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpienaar commented Jul 25, 2025

Uh oh!

mtrofin commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jaddyen commented Jul 29, 2025

Uh oh!

mtrofin commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aniragil commented Aug 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marbre commented Aug 4, 2025

Uh oh!

mtrofin commented Aug 4, 2025

Uh oh!

marbre commented Aug 4, 2025

Uh oh!

mtrofin commented Aug 4, 2025

Uh oh!

simon-camp commented Aug 4, 2025

Uh oh!

Jaddyen commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aniragil commented Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Jaddyen commented Jul 25, 2025 •

edited

Loading

github-actions bot commented Jul 29, 2025 •

edited

Loading

aniragil commented Aug 3, 2025 •

edited

Loading

Jaddyen commented Aug 5, 2025 •

edited

Loading