Skip to content

MaterializeBaseAddresses: stale rationale comment after ConvertFunctions simplification (#96) #99

Description

@fabianlim

MaterializeBaseAddresses.cpp:29-31 explains why the pass keys base addresses positionally:

// Positional keying is the only option available: ConvertFunctions clones the
// function from its FunctionType and copies only visibility, so argument names
// and all argument attributes are already gone by the time this pass runs.

#96 changes the mechanism this describes. ConvertFunctions no longer clones the
function body block-by-block — it builds the func.func from the FunctionType and
then moves the body across with Region::takeBody. So "clones the function" is no
longer what happens, and the consequence the comment draws from it is now partly wrong:

  • Argument attributes — still lost, so the conclusion holds. But the reason changed.
    They are dropped because func::FuncOp::create is given only the FunctionType and
    arg_attrs/res_attrs are never copied over, not because of any cloning.
  • Argument locations — now survive. takeBody carries the entry block and its
    arguments intact, so block-argument locations come across. The old clone created a
    fresh entry block with ttFunc.getLoc() on every argument, which flattened them.

Net effect: positional keying is still the right design and the pass is still correct.
Only the stated rationale is stale. Worth rewording to attribute the loss to the
FunctionType-only construction rather than to cloning, so the next reader does not
go looking for a clone that is no longer there.

Not blocking #96 — the file is not touched by that PR. Filing separately so the
comment does not quietly drift out of sync with the pass it describes.

Found while reviewing #96 (#96 (review)).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentationgood first issueGood for newcomers

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions