This is an archive of the discontinued LLVM Phabricator instance.

[doc]Added examples for generic opcodes
ClosedPublic

Authored by pooja2299 on Jun 26 2021, 5:19 AM.

Details

Summary

Added examples to G_BR, G_BRCOND, G_BRJT, G_BRINDIRECT

Diff Detail

Event Timeline

pooja2299 requested review of this revision.Jun 26 2021, 5:19 AM
pooja2299 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2021, 5:19 AM
xgupta added a subscriber: xgupta.EditedJun 27 2021, 12:19 AM

I would suggest you to take your full time, understand Generic Machine IR thoroughly i.e. know the meaning of every letter in the testcases from where you taken these examples. And then write more descriptive Opcode docs something like as LLVM IR instruction reference https://llvm.org/docs/LangRef.html#instruction-reference.

llvm/docs/GlobalISel/GenericOpcode.rst
732

why 1 not 0 or 2?

740

Why 4 not 3 or 5 or maybe 0?

I would suggest you to take your full time, understand Global Machine IR thoroughly i.e. know the meaning of every letter in the testcases from where you taken these examples. And then write more descriptive Opcode docs something like as LLVM IR instruction reference https://llvm.org/docs/LangRef.html#instruction-reference.

Ok will do that . Since there are short descriptions provided for the Generic opcodes, I thought I have to write short descriptions.

llvm/docs/GlobalISel/GenericOpcode.rst
732

Since other Generic opcodes had examples with mentioning specific registers, I also took an example similar to that . For eg: the example of G_PHI

xgupta added reviewers: aemerson, paquette, arsenm.EditedJun 27 2021, 12:55 AM

Ok will do that . Since there are short descriptions provided for the Generic opcodes, I thought I have to write short descriptions.

I think more description will be better, beginner may find it difficult to understand without details.
Active developers might have more thoughts on how much you should extend it :)

pooja2299 added a comment.EditedJun 27 2021, 1:09 AM

Ok will do that . Since there are short descriptions provided for the Generic opcodes, I thought I have to write short descriptions.

I think more description will be better, beginner may find it difficult to understand without details.
Active developers might have more thoughts on how much you should extend it :)

okk. @paquette @aemerson @arsenm @gandhi21299 @kbarton looking forward for your suggestions.

I think that @xgupta's suggestions are a good idea. I would recommend changing any unnamed vregs (e.g. %0, %2, etc.) to named values.

llvm/include/llvm/Target/GenericOpcodes.td has the source for these. There operand lists may have some more descriptive names that you can reuse. E.g.:

// Generic branch to jump table entry
def G_BRJT : GenericInstruction {
  let OutOperandList = (outs);
  let InOperandList = (ins ptype0:$tbl, unknown:$jti, type1:$idx);
  let hasSideEffects = false;
  let isBranch = true;
  let isTerminator = true;
  let isBarrier = true;
  let isIndirectBranch = true;
}

The input names here aren't super descriptive, but they give a better hint than what we have right now.

Ultimately, I think it would be nice if the documentation was written in this style:

G_ADD
^^^^^^

Adds %src1 to %src0 and stores the result in %dst.

.. code-block:: none

  %dst:_(s32) = G_ADD %src0:_(s32), %src1:_(s32)
llvm/docs/GlobalISel/GenericOpcode.rst
748
756

We could add more descriptive names here, like @xgupta suggested.

I just copied these from GenericOpcodes.td. (Although I changed "$tbl" to "ptr" here because I think that's more beginner-friendly.)

I think that @xgupta's suggestions are a good idea. I would recommend changing any unnamed vregs (e.g. %0, %2, etc.) to named values.

llvm/include/llvm/Target/GenericOpcodes.td has the source for these. There operand lists may have some more descriptive names that you can reuse. E.g.:

// Generic branch to jump table entry
def G_BRJT : GenericInstruction {
  let OutOperandList = (outs);
  let InOperandList = (ins ptype0:$tbl, unknown:$jti, type1:$idx);
  let hasSideEffects = false;
  let isBranch = true;
  let isTerminator = true;
  let isBarrier = true;
  let isIndirectBranch = true;
}

The input names here aren't super descriptive, but they give a better hint than what we have right now.

Thanks a lot! This was very helpful.

Ultimately, I think it would be nice if the documentation was written in this style:

G_ADD
^^^^^^

Adds %src1 to %src0 and stores the result in %dst.

.. code-block:: none

  %dst:_(s32) = G_ADD %src0:_(s32), %src1:_(s32)

Ok, I will keep this in mind.

aemerson added inline comments.Jun 29 2021, 4:08 PM
llvm/docs/GlobalISel/GenericOpcode.rst
732

why 1 not 0 or 2?

We actually begin our block numbering at 1.

740

I suggest %cond or %condition for this

arsenm added inline comments.Jun 29 2021, 5:56 PM
llvm/docs/GlobalISel/GenericOpcode.rst
732

This isn't true, there's just currently some oddity that ends up emitting an extra block in IRTranslator

pooja2299 added inline comments.Jul 4 2021, 11:16 PM
llvm/docs/GlobalISel/GenericOpcode.rst
740

ok thanks @aemerson

756

Sure, I will change the unamed vregs to more descriptive names.

pooja2299 updated this revision to Diff 356461.Jul 5 2021, 3:23 AM

Updated with descriptive names of vregs.

reverse ping @pooja2299

sorry ...I have replied to inline comments week before but forgot to click the 'submit' button below. New to phabricator ..so keep on forgetting these small things. 😅

Will be updating other changes soon.

paquette added inline comments.Jul 6 2021, 10:30 AM
llvm/docs/GlobalISel/GenericOpcode.rst
287

minor typo

724

Might was well rename the vregs here too. :)

Edited name of vregs in the given examples

paquette added inline comments.Jul 26 2021, 9:26 AM
llvm/docs/GlobalISel/GenericOpcode.rst
289

I suggest to make it clear that this sentence references the given example.

pooja2299 added inline comments.Jul 28 2021, 10:21 AM
llvm/docs/GlobalISel/GenericOpcode.rst
289

Yes that would be precise, thanks for indicating it!

pooja2299 updated this revision to Diff 362722.Jul 29 2021, 4:51 AM

Updated the required changes for description of G_ADD

paquette accepted this revision.Jul 29 2021, 9:43 AM

LGTM, thank you!

This revision is now accepted and ready to land.Jul 29 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.

Thanks for reviewing this patch!