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

Fill 17.x/ReleaseNotes with my works #789

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Conversation

chapuni
Copy link
Contributor

@chapuni chapuni commented Nov 27, 2023

Feel free to correct, rephrase, and revise this. Thank you,.

Copy link

@ornata ornata left a comment

Choose a reason for hiding this comment

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

Added comments to make English prose more idiomatic.

``MVT``.

* ``Intrinsics.td`` is able to emit definitions of
``TypeSig``. ``IntrinsicEmitter`` doesn't depend on
Copy link

Choose a reason for hiding this comment

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

Suggestion:

IntrinsicEmitter no longer depends on MachineValueTypes.h

The sentence you wrote is correct. But the suggestion is more idiomatic.

Copy link

Choose a reason for hiding this comment

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

Looking at this again, I think this should be split into 3 bullet points:

  • Intrinsics.td now emits TypeSig definitions
  • IntrinsicEmitter no longer depends on TypeSig
  • IntrinsicEmitter no longer depends on MachineValueTypes.h

``TypeSig``. ``IntrinsicEmitter`` doesn't depend on
``MachineValueTypes.h`` anymore.

* ``llvm/CodeGen/MachineValueType.h`` is moved from ``llvm/Support``.
Copy link

Choose a reason for hiding this comment

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

is moved from

Do you mean removed from? Were the types in llvm/CodeGen/MachineValueType.h moved to ValueTypes.td?

Copy link

Choose a reason for hiding this comment

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

Oh, I see now. I think this would be clearer:

  • MachineValueType.h is moved from llvm/Support to llvm/CodeGen.

Changes to building LLVM
------------------------

* ``llvm-min-tblgen`` is internally introduced to build LLVM public
headers. Note that ``llvm-tblgen`` depends on `GenVT.inc` that is
generated by ``llvm-min-tblgen``. Specify the external
Copy link

Choose a reason for hiding this comment

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

How about

Builds using LLVM_TABLEGEN to specify a pre-built tablegen executable should use llvm-tblgen.

I think you can drop the part about llvm-tblgen being a superset of llvm-min-tblgen. It is implied above.

Changes to building LLVM
------------------------

* ``llvm-min-tblgen`` is internally introduced to build LLVM public
Copy link

Choose a reason for hiding this comment

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

"introduced internally" seems more natural than "internally introduced"

Changes to TableGen
-------------------

* Named arguments are supported. Arguments can be specified in the form of
``name=value``.

* Each tablegen backend can declare itself as self-contained with
``llvm::TableGen::Emitter`` as the registry. You don't need to
Copy link

Choose a reason for hiding this comment

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

Prefer "users" to "you" in documentation.

Changes to TableGen
-------------------

* Named arguments are supported. Arguments can be specified in the form of
``name=value``.

* Each tablegen backend can declare itself as self-contained with
``llvm::TableGen::Emitter`` as the registry. You don't need to
append entries and options to ``TableGen.cpp``. For now, It is
Copy link

Choose a reason for hiding this comment

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

s/It is/it is/

@chapuni
Copy link
Contributor Author

chapuni commented Nov 28, 2023

@ornata Thanks for the review. I have followed your suggestions.

I am ready, @tru could you cherry-pick this please?

@tru tru merged commit b3a30dc into llvm:release/17.x Nov 28, 2023
13 of 14 checks passed
@chapuni chapuni deleted the relnotes/17 branch November 28, 2023 13:31
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

Successfully merging this pull request may close these issues.

3 participants