This is an archive of the discontinued LLVM Phabricator instance.

Differential D101227

[DOCS] Added example for G_EXTRACT and G_INSERT
ClosedPublic

Authored by sushmaunnibhavi on Apr 24 2021, 12:57 AM.

Details

Reviewers
xgupta
arsenm
gargaroff
Commits
rGe4eec519370b: [DOCS] Added example for G_EXTRACT and G_INSERT

Diff Detail

Event Timeline

sushmaunnibhavi requested review of this revision.Apr 24 2021, 12:57 AM
sushmaunnibhavi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2021, 12:57 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B100740: Diff 340248.Apr 24 2021, 1:37 AM
xgupta added a comment.Apr 24 2021, 1:39 AM

Thanks, @sushmaunnibhavi for the patch!

I think the examples you add are for https://llvm.org/docs/GlobalISel/GenericOpcode.html#vector-specific-operations.

For scalar one you should look at /llvm-project/llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-extract.mir and /llvm-project/llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-insert.mir.

xgupta retitled this revision from Added example for G_EXTRACT and G_INSERT [DOCS] to [DOCS] Added example for G_EXTRACT and G_INSERT.Apr 24 2021, 1:42 AM
xgupta added reviewers: arsenm, gargaroff.
Herald added a subscriber: wdng. · View Herald TranscriptApr 24 2021, 1:42 AM
sushmaunnibhavi updated this revision to Diff 340250.Apr 24 2021, 1:48 AM
Harbormaster completed remote builds in B100742: Diff 340250.Apr 24 2021, 1:49 AM
xgupta added a comment.Apr 24 2021, 2:13 AM

@sushmaunnibhavi, I can't apply your updated patch. I think you should first rebase your branch with LLVM upstream main.
There is a recent tutorial (https://reviews.llvm.org/D100714) on the contribution that also talks about arc uses.

xgupta added a comment.Apr 24 2021, 3:00 AM

Or if you are updating diff from Phabticator GUI, make sure you follow the steps mentions here -https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.
But I think using arc (command line) is a better way.

sushmaunnibhavi updated this revision to Diff 340258.Apr 24 2021, 3:52 AM
In D101227#2714434, @xgupta wrote:

Or if you are updating diff from Phabticator GUI, make sure you follow the steps mentions here -https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.
But I think using arc (command line) is a better way.

Yes I did it now, can you please check?

Harbormaster completed remote builds in B100745: Diff 340258.Apr 24 2021, 3:52 AM
sushmaunnibhavi updated this revision to Diff 340260.Apr 24 2021, 4:05 AM
Harbormaster completed remote builds in B100746: Diff 340260.Apr 24 2021, 4:05 AM
sushmaunnibhavi updated this revision to Diff 340263.Apr 24 2021, 4:12 AM
xgupta accepted this revision.Apr 24 2021, 4:24 AM

LGTM!

This revision is now accepted and ready to land.Apr 24 2021, 4:24 AM
xgupta added a comment.Apr 24 2021, 4:37 AM

Whenever you make changes to docs, make sure you run make docs-llvm-html before updating the revision.

llvm/docs/GlobalISel/GenericOpcode.rst
190

A line break is needed between these two lines

199

same here and %5 should be shifted two keystrokes right for indentation.

Harbormaster completed remote builds in B100749: Diff 340263.Apr 24 2021, 4:51 AM
sushmaunnibhavi added a comment.Apr 24 2021, 6:05 AM
In D101227#2714474, @xgupta wrote:

Whenever you make changes to docs, make sure you run make docs-llvm-html before updating the revision.

Okay I will do it.

sushmaunnibhavi updated this revision to Diff 340426.Apr 25 2021, 9:31 PM
Harbormaster completed remote builds in B100859: Diff 340426.Apr 25 2021, 9:31 PM
sushmaunnibhavi updated this revision to Diff 340427.Apr 25 2021, 9:38 PM
Harbormaster completed remote builds in B100860: Diff 340427.Apr 25 2021, 10:39 PM
gargaroff added inline comments.Apr 29 2021, 11:44 PM
llvm/docs/GlobalISel/GenericOpcode.rst
191

I think you should add the type for %2 to make it clear that this changes the type. I.e. %2:_(s64)

200

And same here.

sushmaunnibhavi updated this revision to Diff 342138.May 1 2021, 5:16 AM
Harbormaster completed remote builds in B102093: Diff 342138.May 1 2021, 5:53 AM
xgupta added a comment.May 3 2021, 12:30 AM

@gargaroff Is the patch looks good?

gargaroff accepted this revision.May 3 2021, 12:35 AM

With the nit fixed, this should be good to go. Thanks!

llvm/docs/GlobalISel/GenericOpcode.rst
200

Add the type for %0 as well to make it clear that this is the same type as the result.

sushmaunnibhavi updated this revision to Diff 342974.May 5 2021, 3:00 AM

Added type for %0 in G_INSERT operation

This revision was landed with ongoing or failed builds.May 5 2021, 3:19 AM
Closed by commit rGe4eec519370b: [DOCS] Added example for G_EXTRACT and G_INSERT (authored by • sush0710, committed by xgupta). · Explain Why
This revision was automatically updated to reflect the committed changes.
xgupta added a commit: rGe4eec519370b: [DOCS] Added example for G_EXTRACT and G_INSERT.
xgupta added a comment.May 5 2021, 3:20 AM

Congratulations

Harbormaster completed remote builds in B102691: Diff 342974.May 5 2021, 3:35 AM

Revision Contents

PathSize
llvm/
docs/
GlobalISel/
8 lines

Diff 342978

llvm/docs/GlobalISel/GenericOpcode.rst

Show First 20 Lines • Show All 180 Lines • ▼ Show 20 Lines
G_EXTRACT G_EXTRACT
^^^^^^^^^ ^^^^^^^^^
Extract a register of the specified size, starting from the block given by Extract a register of the specified size, starting from the block given by
index. This will almost certainly be mapped to sub-register COPYs after index. This will almost certainly be mapped to sub-register COPYs after
register banks have been selected. register banks have been selected.
.. code-block:: none
xguptaUnsubmitted
Not Done
ReplyInline Actions

A line break is needed between these two lines

xgupta: A line break is needed between these two lines
%3:_(s32) = G_EXTRACT %2:_(s64), 32
gargaroffUnsubmitted
Not Done
ReplyInline Actions

I think you should add the type for %2 to make it clear that this changes the type. I.e. %2:_(s64)

gargaroff: I think you should add the type for `%2` to make it clear that this changes the type. I.e. `%2…
G_INSERT G_INSERT
^^^^^^^^ ^^^^^^^^
Insert a smaller register into a larger one at the specified bit-index. Insert a smaller register into a larger one at the specified bit-index.
.. code-block:: none
xguptaUnsubmitted
Not Done
ReplyInline Actions

same here and %5 should be shifted two keystrokes right for indentation.

xgupta: same here and %5 should be shifted two keystrokes right for indentation.
%2:_(s64) = G_INSERT %0:(_s64), %1:_(s32), 0
gargaroffUnsubmitted
Not Done
ReplyInline Actions

And same here.

gargaroff: And same here.
gargaroffUnsubmitted
Not Done
ReplyInline Actions

Add the type for %0 as well to make it clear that this is the same type as the result.

gargaroff: Add the type for %0 as well to make it clear that this is the same type as the result.
G_MERGE_VALUES G_MERGE_VALUES
^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^
Concatenate multiple registers of the same size into a wider register. Concatenate multiple registers of the same size into a wider register.
The input operands are always ordered from lowest bits to highest: The input operands are always ordered from lowest bits to highest:
.. code-block:: none .. code-block:: none
▲ Show 20 Lines • Show All 619 Lines • Show Last 20 Lines