Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InjectExtensions should support multiple patches per file #1

Open
PhilippvK opened this issue Aug 31, 2023 · 3 comments
Open

InjectExtensions should support multiple patches per file #1

PhilippvK opened this issue Aug 31, 2023 · 3 comments
Assignees

Comments

@PhilippvK
Copy link

There is currently no way to have two sets of START/END markers in the same source file (at least not when using the File Name as tag)

It would be great if each set of markers would accept an optional label to differentiate between multiple sections.

@thomasgoodfellow thomasgoodfellow self-assigned this Sep 11, 2023
@thomasgoodfellow
Copy link
Contributor

thomasgoodfellow commented Sep 14, 2023

The use of the filename as a tag has already proved regrettable when refactoring between LLVM versions led to a filename change. Really these markers should be purely discriptive and located by a search. The current approach was driven largely by the fear that it would be too expensive but having now tried this it showed once again that "premature optimisation is the root of all evil" since the first naive python approach tried scanned 13117125 lines read from 50462 candidate files in just over four seconds on a fairly modest server.

So a proposal for a better approach is to change the YAML index file that drives the inject_extensions script so the file fragment paths have the marker as the key, e.g.:

  • name: s4emac
    description: MAC
    patches:
    • Instruction-Layout-TD: s4emac/RISCVInstrInfo_ISAX_S4E_MAC.td
    • Builtins-Table: s4emac/S4E_MAC.builtins.cpp

then the mark "Builtins-Table - INSERTION_START" is found in ./clang/lib/CodeGen/CGBuiltin.cpp, "Instruction-Layout - INSERTION_START" is found in ./llvm/lib/Target/RISCV/RISCVInstrInfo.td, etc. Multiple marks in the same file will be supported, as should applying multiple patch sets, e.g. adding two disjoint extensions.

This would reduce the coupling of the patching script to both the YAML contents and the current LLVM layout. Does it look as if fits your needs?

@thomasgoodfellow
Copy link
Contributor

Adding the trial mark-finder script referred to in previous comment as an aide-memoire
findmarks.txt

@thomasgoodfellow thomasgoodfellow transferred this issue from DLR-SE/extensible-compiler Oct 16, 2023
@PhilippvK
Copy link
Author

The new injection script added in 467b68f#diff-e58a38fb7894b1e0def805c6af3e4cdc1e297e47977a03cae8ac081ec90fb471 suits my needs. However the code needs to be cleaned up (remove commented old code) and missing features should be implemented or dropped (See NotImplementedErrors)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants