Skip to content

Mark inline locations with annotation.#5150

Merged
asl merged 5 commits into
p4lang:mainfrom
asl:inlinedAt
Mar 5, 2025
Merged

Mark inline locations with annotation.#5150
asl merged 5 commits into
p4lang:mainfrom
asl:inlinedAt

Conversation

@asl

@asl asl commented Feb 25, 2025

Copy link
Copy Markdown
Contributor

Add policy for SimplifyControlFlow to remove / not remove such marked blocks

@asl asl requested review from ChrisDodd, fruffy and vlstill February 25, 2025 19:29
@asl

asl commented Feb 25, 2025

Copy link
Copy Markdown
Contributor Author

Currently the inlined locations are not well preserved: indeed inliners create a BlockStatement with inlined function / action body and attach original call expression location to it. However, pretty soon SimplifyControlFlow kills such blocks and there is no way to reconstruct original inline location: it looks like we're doing just textual inclusion via preprocessor.

In DWARF we're having a dedicated inlined at marker that could be indicated that "scope A is inlined here at scope B" and use this to reconstruct original functions even after inlining.

Here we are doing something similar:

  • We mark such BlockStatement's with an inlinedAt attribute (we also copy source location to this attribute for the sake of completeness of all information).
  • It turns that SimplifyControlFlow does not touch blocks with annotations (mostly intended for @id and similar annotations), so they are preserved by default

However, this might interfere with some midend passes, namely action synthesis. So, we add additional frontend policy to control the folding of @inlinedAt, the backends could decide on the intended behavior here. Midend pipeline is expected to signal intended functionality while instantiating SimplifyControlFlow pass.

@asl

asl commented Feb 25, 2025

Copy link
Copy Markdown
Contributor Author

Some fixes for source locations while there: somehow they were attached to arguments, that were dropped afterwards, not the variables created to hold parameter values.

return !(
a->name == IR::Annotation::nameAnnotation ||
(a->name == IR::Annotation::noWarnAnnotation && a->getSingleString() == "unused"));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe should have a frontend policy here to control which annotations are preserved and which are removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Interesting enough, the filtering of annotations are only for actions and not for functions currently. Likely some uniformity would be great.

Comment thread frontends/p4/frontend.h Outdated

/// Indicates whether control flow should fold blocks marked with @inlinedAt annotation
/// @returns Defaults to true
virtual bool foldInlinedAt() const { return true; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

perhaps this should be more generic, with a list of annotations to allow folding for?

Comment thread ir/annotations.cpp Outdated
const cstring IR::Annotation::fieldListAnnotation = "field_list"_cs;
const cstring IR::Annotation::debugLoggingAnnotation = "__debug"_cs;
const cstring IR::Annotation::disableOptimizationAnnotation = "disable_optimization"_cs;
const cstring IR::Annotation::inlinedAtAnnotation = "inlinedAt"_cs;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: inlinedAt seems a bit of a misnomer here -- perhaps should be inlinedFrom?

@asl asl Feb 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I reused the terminology from DWARF standard. inlinedAt marks the place. And from is an argument. But I do not have a strong preference, maybe because it is an annotation on the whole block, inlinedFrom would be better, yes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was initially confused with inlinedAt so I also think inlinedFrom is a better term.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was renamed to inlinedFrom. It is more suitable for the version with the name (original version did not have name so marked essentially the inlined location)

@asl asl force-pushed the inlinedAt branch 2 times, most recently from a37bc41 to 847366c Compare February 25, 2025 23:18
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 28, 2025
Comment thread testdata/p4_16_samples_outputs/xor_test-frontend.p4 Outdated
Comment thread ir/annotations.cpp Outdated
const cstring IR::Annotation::fieldListAnnotation = "field_list"_cs;
const cstring IR::Annotation::debugLoggingAnnotation = "__debug"_cs;
const cstring IR::Annotation::disableOptimizationAnnotation = "disable_optimization"_cs;
const cstring IR::Annotation::inlinedAtAnnotation = "inlinedAt"_cs;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was initially confused with inlinedAt so I also think inlinedFrom is a better term.

asl added 5 commits March 4, 2025 10:59
…Flow

to remove / not remove such marked blocks

Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl asl enabled auto-merge March 4, 2025 19:07
@asl asl added this pull request to the merge queue Mar 5, 2025
Merged via the queue into p4lang:main with commit bf2aa7b Mar 5, 2025
@asl asl deleted the inlinedAt branch March 5, 2025 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants