This is an archive of the discontinued LLVM Phabricator instance.

Differential D107630

[docs][GlobalISel]Adding info for G_JUMP_TABLE generic opcode
ClosedPublic

Authored by pooja2299 on Aug 6 2021, 2:52 AM.

Details

Reviewers
gandhi21299
kbarton
aemerson
paquette
arsenm
xgupta
Commits
rG843da6f4085c: [docs][GlobalISel]Adding info for G_JUMP_TABLE generic opcode
Summary

Added description of jump table and G_JUMP_TABLE opcode.

Diff Detail

Event Timeline

pooja2299 created this revision.Aug 6 2021, 2:52 AM
Herald added a subscriber: rovka. · View Herald TranscriptAug 6 2021, 2:52 AM
pooja2299 requested review of this revision.Aug 6 2021, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 2:52 AM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
Harbormaster completed remote builds in B118339: Diff 364736.Aug 6 2021, 3:30 AM
paquette added a comment.Aug 10 2021, 2:02 PM

Added some stylistic nits.

llvm/docs/GlobalISel/GenericOpcode.rst
825–834

Maybe a little more concise?

826
  • Maybe split this sentence up?
  • I don't think parens are necessary here.
829
  • Remind the reader about what "it" is
  • Noun-ify G_BRJT
pooja2299 updated this revision to Diff 365799.Aug 11 2021, 10:45 AM

Updated with required changes with sentences of the G_JUMP_TABLE documentation.

pooja2299 added a comment.Aug 11 2021, 10:48 AM
In D107630#2938130, @paquette wrote:

Added some stylistic nits.

Thanks for reviewing. If any other changes are required I would be happy to change.

Harbormaster completed remote builds in B119109: Diff 365799.Aug 11 2021, 11:15 AM
paquette added a comment.Aug 13 2021, 3:15 PM

Did some git archaeology to give some better feedback. :)

llvm/docs/GlobalISel/GenericOpcode.rst
833

When it's a pointer, we should use pN, where N is the address space.

I think 0 is the default, so you can probably just use that.

835

I took a look at the commit that added G_JUMP_TABLE to GenericOpcodes.td (d133c1592560edb77958492a77e4e871b21a9d52)

The message says:

[GlobalISel] Add a G_JUMP_TABLE opcode.

This opcode generates a pointer to the address of the jump table
specified by the source operand, which is a jump table index.

It will be used in conjunction with an upcoming G_BRJT opcode to support
jump table codegen with GlobalISel.

Differential Revision: https://reviews.llvm.org/D63111

So I think that we should basically use that description here. :)

pooja2299 added inline comments.Aug 14 2021, 11:11 PM
llvm/docs/GlobalISel/GenericOpcode.rst
833

ok sure

835

ok will do that

pooja2299 updated this revision to Diff 366861.Aug 17 2021, 4:47 AM

Improved the description of G_JUMP_TABLE

Harbormaster completed remote builds in B119876: Diff 366861.Aug 17 2021, 6:06 AM
paquette accepted this revision.Aug 17 2021, 9:38 AM

LGTM

This revision is now accepted and ready to land.Aug 17 2021, 9:38 AM
xgupta added a subscriber: xgupta.Aug 17 2021, 10:19 AM
xgupta added inline comments.
llvm/docs/GlobalISel/GenericOpcode.rst
836

minor nit: I think "Source operand must be a jump table index." seems redundant here as you already said above "The source operand is a jump table index."

pooja2299 updated this revision to Diff 367521.Aug 19 2021, 8:59 AM

Removed the redundant line.

Harbormaster completed remote builds in B120351: Diff 367521.Aug 19 2021, 9:39 AM
aemerson accepted this revision.Aug 19 2021, 5:00 PM
aemerson added inline comments.
llvm/docs/GlobalISel/GenericOpcode.rst
826–827

I don't think this sentence really fits in this doc. If the user doesn't know what a jump table is then it's better to go read about it in a more detailed source that here. Otherwise, LGTM.

pooja2299 updated this revision to Diff 368121.Aug 23 2021, 8:44 AM

updated with requiredd minor changes

Harbormaster completed remote builds in B120803: Diff 368121.Aug 23 2021, 10:01 AM
xgupta accepted this revision.Aug 29 2021, 10:23 AM
Closed by commit rG843da6f4085c: [docs][GlobalISel]Adding info for G_JUMP_TABLE generic opcode (authored by pooja2299). · Explain WhyAug 30 2021, 12:14 PM
This revision was automatically updated to reflect the committed changes.
pooja2299 added a commit: rG843da6f4085c: [docs][GlobalISel]Adding info for G_JUMP_TABLE generic opcode.

Revision Contents

PathSize
llvm/
docs/
GlobalISel/
12 lines

Diff 369516

llvm/docs/GlobalISel/GenericOpcode.rst

Show First 20 Lines • Show All 816 Lines • ▼ Show 20 Lines
.. code-block:: none .. code-block:: none
G_BRJT %ptr(p0), %jti, %idx(s64) G_BRJT %ptr(p0), %jti, %idx(s64)
G_JUMP_TABLE G_JUMP_TABLE
^^^^^^^^^^^^ ^^^^^^^^^^^^
.. caution:: Generates a pointer to the address of the jump table specified by the source
operand. The source operand is a jump table index.
paquetteUnsubmitted
Not Done
ReplyInline Actions
This generic jump table opcode generates a pointer to the address of the jump
- table specified by the source operand, which is a jump table index. (A jump
+ table specified by the source operand. The source operand is a jump table index. A jump
table is an array of addresses the CPU should jump to based on certain
  • Maybe split this sentence up?
  • I don't think parens are necessary here.
paquette: - Maybe split this sentence up? - I don't think parens are necessary here.
G_JUMP_TABLE can be used in conjunction with G_BRJT to support jump table
aemersonUnsubmitted
Not Done
ReplyInline Actions

I don't think this sentence really fits in this doc. If the user doesn't know what a jump table is then it's better to go read about it in a more detailed source that here. Otherwise, LGTM.

aemerson: I don't think this sentence really fits in this doc. If the user doesn't know what a jump table…
codegen with GlobalISel.
paquetteUnsubmitted
Not Done
ReplyInline Actions
conditions.)
- It can be used in conjunction with G_BRJT opcode to support jump table codegen
+ G_JUMP_TABLE can be used in conjunction with G_BRJT to support jump table codegen
with GlobalISel.
  • Remind the reader about what "it" is
  • Noun-ify G_BRJT
paquette: - Remind the reader about what "it" is - Noun-ify G_BRJT
.. code-block:: none
%dst:_(p0) = G_JUMP_TABLE %jump-table.0
paquetteUnsubmitted
Not Done
ReplyInline Actions
.. code-block:: none
- %dst:_(s32) = G_JUMP_TABLE %jump-table.0
+ %dst:_(p0) = G_JUMP_TABLE %jump-table.0
The above example shows assignment of pointer to jump table of given index.

When it's a pointer, we should use pN, where N is the address space.

I think 0 is the default, so you can probably just use that.

paquette: When it's a pointer, we should use `pN`, where `N` is the address space. I think 0 is the…
pooja2299AuthorUnsubmitted
Done
ReplyInline Actions

ok sure

pooja2299: ok sure
The above example generates a pointer to the source jump table index.
paquetteUnsubmitted
Not Done
ReplyInline Actions
^^^^^^^^^^^^
- This generic jump table opcode generates a pointer to the address of the jump
+ Generates a pointer to the address of the jump
table specified by the source operand, which is a jump table index. (A jump

Maybe a little more concise?

paquette: Maybe a little more concise?
paquetteUnsubmitted
Not Done
ReplyInline Actions
%dst:_(s32) = G_JUMP_TABLE %jump-table.0
- The above example shows assignment of pointer to jump table of given index.
- Source operand must be a jump table index.
+ The above example generates a pointer to the source jump table index.Source operand must be a jump table index.

I took a look at the commit that added G_JUMP_TABLE to GenericOpcodes.td (d133c1592560edb77958492a77e4e871b21a9d52)

The message says:

[GlobalISel] Add a G_JUMP_TABLE opcode.

This opcode generates a pointer to the address of the jump table
specified by the source operand, which is a jump table index.

It will be used in conjunction with an upcoming G_BRJT opcode to support
jump table codegen with GlobalISel.

Differential Revision: https://reviews.llvm.org/D63111

So I think that we should basically use that description here. :)

paquette: I took a look at the commit that added G_JUMP_TABLE to GenericOpcodes.td…
pooja2299AuthorUnsubmitted
Done
ReplyInline Actions

ok will do that

pooja2299: ok will do that
I found no documentation for this instruction at the time of writing.
xguptaUnsubmitted
Not Done
ReplyInline Actions

minor nit: I think "Source operand must be a jump table index." seems redundant here as you already said above "The source operand is a jump table index."

xgupta: minor nit: I think "Source operand must be a jump table index." seems redundant here as you…
G_INTRINSIC, G_INTRINSIC_W_SIDE_EFFECTS G_INTRINSIC, G_INTRINSIC_W_SIDE_EFFECTS
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Call an intrinsic Call an intrinsic
The _W_SIDE_EFFECTS version is considered to have unknown side-effects and The _W_SIDE_EFFECTS version is considered to have unknown side-effects and
as such cannot be reordered across other side-effecting instructions. as such cannot be reordered across other side-effecting instructions.
▲ Show 20 Lines • Show All 70 Lines • Show Last 20 Lines