This is an archive of the discontinued LLVM Phabricator instance.

[doc][GlobalISel]Improving generic opcodes for memory operations
AcceptedPublic

Authored by pooja2299 on Aug 18 2021, 12:16 PM.

Details

Summary

Adding examples for each of the generic opcodes involved in memory operations. Yet to add examples for some of the opcodes.

Diff Detail

Event Timeline

pooja2299 created this revision.Aug 18 2021, 12:16 PM
pooja2299 requested review of this revision.Aug 18 2021, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 12:16 PM
pooja2299 added inline comments.Aug 18 2021, 12:30 PM
llvm/docs/GlobalISel/GenericOpcode.rst
680–681

I am not aware that with what reference previous author has written this example, so needed guidance before changing it .

705

Should I add the description of below example instead of this line?

paquette added inline comments.Aug 19 2021, 2:15 PM
llvm/docs/GlobalISel/GenericOpcode.rst
656

I don't think you need to change this.

662

I think for this example, you want to use G_ZEXTLOAD.

You'll also want a memoperand.

E.g.

%load:_(s64) = G_LOAD %ptr(p0) :: (load (s32)) ; High 32 bits are undefined.
%load:_(s64) = G_SEXTLOAD %ptr(p0) :: (load (s32)) ; High 32 bits are sign-extended.
%load:_(s64) = G_ZEXTLOAD %ptr(p0) :: (load (s32)) ; High 32 bits are zero-extended.
673–679

Can we change GEP to G_PTR_ADD?

677

I think I'd drop this part and only have G_INDEXED_LOAD examples. Or, I'd make it clear that these are being combined to a G_INDEXED_LOAD.

I'd also add an example for the post-indexed G_INDEXED_LOAD.

705

I think it's fine as is.

709

I don't think we need to show the original G_STORE + G_PTR_ADD in the example. Only the G_INDEXED_STORE.

Edited the redundant examples

pooja2299 updated this revision to Diff 369262.Aug 28 2021, 8:02 AM

replaced GEP with G_PTR_ADD

ping

Hello. Was busy with my placements and final year project. Should I commit this patch or does it need any further changes?

LGTM, appreciate your efforts!

gandhi21299 accepted this revision.Oct 19 2021, 9:47 AM
This revision is now accepted and ready to land.Oct 19 2021, 9:47 AM

FYI, it seems like a branch containing this patch was accidentally pushed to GitHub: https://github.com/llvm/llvm-project/tree/arcpatch-D108319

FYI, it seems like a branch containing this patch was accidentally pushed to GitHub: https://github.com/llvm/llvm-project/tree/arcpatch-D108319

Apologies for that. I will revert the pull request.