[PATCH] D108319: [doc][GlobalISel]Improving generic opcodes for memory operations

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:656
 
-Generic load. Expects a MachineMemOperand in addition to explicit
+Generic load expects a MachineMemOperand in addition to explicit
 operands. If the result size is larger than the memory size, the
----------------
I don't think you need to change this.


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:662
+
+  %dst(s8) = G_LOAD %ptr
+  %dst(s32) = G_ZEXT %dst(s8)
----------------
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.
```


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:672
 
-Generic indexed load. Combines a GEP with a load. $newaddr is set to $base + $offset.
+Generic indexed load opcode combines a GEP with a load. 
+
----------------
Can we change GEP to G_PTR_ADD?


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:676
+
+  $addr = G_PTR_ADD $base, $offset
+  [...]
----------------
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.


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:709
 
 Combines a store with a GEP. See description of G_INDEXED_LOAD for indexing behaviour.
 
----------------
pooja2299 wrote:
> Should I add the description of below example instead of this line?
I think it's fine as is.


================
Comment at: llvm/docs/GlobalISel/GenericOpcode.rst:713
+
+  G_STORE $val, $base
+  [...]
----------------
I don't think we need to show the original G_STORE + G_PTR_ADD in the example. Only the G_INDEXED_STORE.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108319/new/

https://reviews.llvm.org/D108319