Add code:yytarget_elem to syntax files. #537

caffe3 · 2025-03-01T17:35:49Z

code:yytarget_elem specifies the type used for the element types in the computed goto tables.

include/syntax/c

caffe3 · 2025-03-02T10:46:18Z

Should I make another syntax variable for relative gotos as well i.e. gen_gocp_goto ?

code:yytarget_elem specifies the how the table elements for computed goto tables are generated. If cgoto:relative = 1, then the table entries contain an offset from a calculated base address. Bootstrap code has been regenerate.

skvadrik · 2025-03-02T15:42:54Z

Should I make another syntax variable for relative gotos as well i.e. gen_gocp_goto ?

You mean another syntax configuration for the goto statement that uses table elements? Yes please. Something like code:yytarget_goto = topindent <here goes the definition...> nl;

skvadrik

Thanks! Looks good overall, just a few suggestions how to polish it.

skvadrik · 2025-03-02T15:45:25Z

include/syntax/c

@@ -441,3 +441,5 @@ code:yylessthan =
 code:yybm_filter = yych " & ~0xFF";

 code:yybm_match = yybm "[" offset "+" yych "] & " mask;
+
+code:yytarget_elem = (.cgoto.relative ? "(int)((char*)&&" : "&&") label (.cgoto.relative ? " - (char*)&&" base ")");


I think it will be a bit cleaner if you totally split the two cases, like this:

code:yytarget_elem = (.cgoto.relative ? "(int)((char*)&&" label " - (char*)&&" base ")" : "&&" label);

skvadrik · 2025-03-02T15:46:33Z

src/codegen/pass2_generate.cc

@@ -177,6 +177,27 @@ class GenEnumElem : public RenderCallback {
    FORBID_COPY(GenEnumElem);
 };

+class GenYytargetElem : public RenderCallback {


Just GenTargetElem to match the rest of the names that don't have YY.

skvadrik · 2025-03-02T15:47:45Z

src/codegen/pass2_generate.cc

+        case StxVarId::BASE: os << base; break;
+        case StxVarId::LABEL: os << label; break;


Formatting: in a simple switch where each case fits on one line we do indent cases.

skvadrik · 2025-03-02T15:53:14Z

src/codegen/pass2_generate.cc

+    const std::string base = buf.str(prefix).label(base_label).flush();
+    const std::string lbl = buf.str(prefix).label(label).flush();


Instead of creating these temporary strings with unnecessarily long lifetime, can you save options and label references in GenTargetElem and output them directly?

caffe3 · 2025-03-02T18:43:58Z

Should I make another syntax variable for relative gotos as well i.e. gen_gocp_goto ?

You mean another syntax configuration for the goto statement that uses table elements? Yes please. Something like code:yytarget_goto = topindent <here goes the definition...> nl;

Do you mean to have something like:

code:yytarget_goto = topindent "goto " (.cgoto.relative ? "*((char*)&&" base " + " array "[" index "])" : "*" array "[" index "]") nl ";";

skvadrik · 2025-03-02T21:47:32Z

Do you mean to have something like [...]

Yes, but please use yytarget variable instead of array like we do for yybm and other primitives.

caffe3 · 2025-03-03T04:32:49Z

Do you mean to have something like [...]

Yes, but please use yytarget variable instead of array like we do for yybm and other primitives.

I chose array because the code currently overlaps condition table generation with yytarget. I guess then we should consider make a separate set of syntax for yyctable (which would be useful for relative gotos anyway so we don't have to use the same types for both of them). Do you have any naming preferences for conditions?

skvadrik · 2025-03-03T06:54:51Z

I chose array because the code currently overlaps condition table generation with yytarget. I guess then we should consider make a separate set of syntax for yyctable (which would be useful for relative gotos anyway so we don't have to use the same types for both of them). Do you have any naming preferences for conditions?

Ah, I see why you chose array. But if we'd have to use something more generic than code:yytarget_elem, since it would be strange to use yytarget in configuration name, but not in the variable name. I see two options.

First, sticking with yytarget in both places. As for yyctable, making another set of configurations for it seems like a waste: it's a very niche use case, and only for one language. We can reuse yytarget configurations for yyctable (and substitute yyctable for yytarget variable) with a comment explaining this in the codegen code. This way we leave the options open for later if we want to add separate configs for yyctable.

Second, we can rename code:yytarget_goto and code:yytarget_elem to code:cgoto and code:cgoto_target respectively, move them in the syntax file under code:goto definition and use array variable name. This is more generic and can be reused for any APIs.

What do you think?

caffe3 · 2025-03-03T07:06:57Z

First, sticking with yytarget [...]
Second, [...]

What do you think?

I think the first option could be useful - there's a high chance that when conditions are used, there will be very few conditions, one could in theory set the elem type for conditions as uint8_t.

skvadrik · 2025-03-03T08:29:35Z

I think the first option could be useful - there's a high chance that when conditions are used, there will be very few conditions, one could in theory set the elem type for conditions as uint8_t.

Ok, let's go this way. Should we also add code:type_yyctable then? Could be done in a follow-up commit / PR.

skvadrik requested changes Mar 1, 2025

View reviewed changes

include/syntax/c Outdated Show resolved Hide resolved

caffe3 force-pushed the cgoto-yytarget-elem branch from b5c7d5b to eb98c66 Compare March 2, 2025 10:40

Add code:yytarget_elem to syntax files.

dfe47cc

code:yytarget_elem specifies the how the table elements for computed goto tables are generated. If cgoto:relative = 1, then the table entries contain an offset from a calculated base address. Bootstrap code has been regenerate.

caffe3 force-pushed the cgoto-yytarget-elem branch from eb98c66 to dfe47cc Compare March 2, 2025 10:49

skvadrik requested changes Mar 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code:yytarget_elem to syntax files. #537

Add code:yytarget_elem to syntax files. #537

caffe3 commented Mar 1, 2025

caffe3 commented Mar 2, 2025

skvadrik commented Mar 2, 2025

skvadrik left a comment

skvadrik Mar 2, 2025

skvadrik Mar 2, 2025

skvadrik Mar 2, 2025

skvadrik Mar 2, 2025

caffe3 commented Mar 2, 2025

skvadrik commented Mar 2, 2025

caffe3 commented Mar 3, 2025

skvadrik commented Mar 3, 2025

caffe3 commented Mar 3, 2025

skvadrik commented Mar 3, 2025

		case StxVarId::BASE: os << base; break;
		case StxVarId::LABEL: os << label; break;

		const std::string base = buf.str(prefix).label(base_label).flush();
		const std::string lbl = buf.str(prefix).label(label).flush();

Add code:yytarget_elem to syntax files. #537

Are you sure you want to change the base?

Add code:yytarget_elem to syntax files. #537

Conversation

caffe3 commented Mar 1, 2025

caffe3 commented Mar 2, 2025

skvadrik commented Mar 2, 2025

skvadrik left a comment

Choose a reason for hiding this comment

skvadrik Mar 2, 2025

Choose a reason for hiding this comment

skvadrik Mar 2, 2025

Choose a reason for hiding this comment

skvadrik Mar 2, 2025

Choose a reason for hiding this comment

skvadrik Mar 2, 2025

Choose a reason for hiding this comment

caffe3 commented Mar 2, 2025

skvadrik commented Mar 2, 2025

caffe3 commented Mar 3, 2025

skvadrik commented Mar 3, 2025

caffe3 commented Mar 3, 2025

skvadrik commented Mar 3, 2025