This is an archive of the discontinued LLVM Phabricator instance.

TableGen: Let getAllDerivedDefinitions() numeric order.
ClosedPublic

Authored by chapuni on Mar 12 2023, 7:48 AM.

Details

Summary

Since RK::Recs is sorted by character order, anonymous defs will be
enumerated like this;

  • anonymous_0
  • anonymous_1
  • anonymous_10
  • anonymous_100
  • anonymous_1000
  • ...
  • anonymous_99
  • anonymous_990
  • ...
  • anonymous_999

Some records around each gap might be wrapped around along increase or
decrease of records in middle. Then output order of anonymous defs
might be changed.

Numeric sort is expected to prevent such wrap-arounds.
This can be implemented with StringRef::compare_numeric().

  • ...
  • anonymous_99
  • anonymous_100
  • ...
  • anonymous_999
  • anonymous_1000
  • ...

Although it would be simpler to change the comparator of Recs as numeric,
I don't do, since;

  • I am afraid if numeric comparator is not lightweit.
  • It triggers behavior change (mentioned above)

Diff Detail

Event Timeline

chapuni created this revision.Mar 12 2023, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:48 AM
chapuni requested review of this revision.Mar 12 2023, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:48 AM

I guess this change would affect LLVM CodeGen.
(When I tested on the entire tree, only llvm/test/CodeGen complained)

I wonder whether I could restore an exception in RegisterTuples.

I have optionally suppressed numeric sort on RegisterTuples

in CodeGenRegBank, or several tests would fail due to behavior change.

It might be a change but it shouldn't be wrong. What do the changes look like?

chapuni updated this revision to Diff 506281.Mar 18 2023, 2:53 AM
chapuni edited the summary of this revision. (Show Details)
  • getAllDerivedDefinitions() always sort numeric.
  • Fix up 6 tests.

@arsenm I have confirmed 6 of tests don't change actual behavior.

I'm not clear what problem we're trying to solve in practice. Many of the emitters do their own sort of the Records.

Should anonymous records even be in the name based map? What would the impact be of storing them in a separate vector?

I have created D146519 as an experiment.

@craig.topper I tried isolating anonymous defs from RK::Defs.
I gave up for now, since seems TGParser::ParseDefm() relies on lexical name of anonymous records.

As another experiment, I tried in RK::getAllDerivedDefinitions()

  1. Extract and push anonymous defs as registration order.
  2. Extract and push named defs from RK::Defs (as char order)

It worked. I could implement RK::AnonDefs as vector.

arsenm accepted this revision.Mar 30 2023, 11:18 AM

LGTM. I would assume this is the ordering anonymous records would get and it shouldn't really matter

This revision is now accepted and ready to land.Mar 30 2023, 11:18 AM
This revision was landed with ongoing or failed builds.Mar 30 2023, 2:01 PM
This revision was automatically updated to reflect the committed changes.