[PATCH] D61289: [globalisel] Add G_SEXT_INREG

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

Perfect. What about G_ZEXT_INREG since it is more efficient to narrow scalar G_ZEXT_INREG then bitwise instruction with some bit mask?



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:698
     return Legalized;
+  case TargetOpcode::G_SEXT_INREG: {
+    if (TypeIdx != 0)
----------------
NarrowScalar is good candidate for separate patch. Reason is that artifact combiner currently can not handle test that would use  G_SEXT_INREG since it will have chained artifacts.  G_SEXT will be  G_UNMERGE-d, G_UNMERGE has to wait for: 
 - G_SEXT to be combined into G_SEXT_INREG,
 - and then for G_SEXT_INREG to be narrowScalared into sequence of instructions that will end with G_MERGE_VALUES. 
Only at this point G_UNMERGE can be combined.




================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:708
+    // don't need to break it apart.
+    if (NarrowTy.getScalarSizeInBits() >= SizeInBits) {
+      Observer.changingInstr(MI);
----------------
What is the idea behind this block, we generate two instructions with types larger then NarrowTy? G_TRUNC and G_SEXT that are generated have to be narrowScalared and then we have a few more merge/unmerge combines to do.  Also LegalizerHelper does not know how to narrow scalar G_TRUNC and G_SEXT at the moment.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:756
+
+    // Do the operation on each small part.
+    for (int i = 0; i < NumParts; ++i) {
----------------
This loop works for `NarrowTy.getScalarSizeInBits() >= SizeInBits` as well.
`NarrowTy.getScalarSizeInBits()` -> `NarrowSize`



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:767
+        }
+        DstRegs.push_back(MIRBuilder
+                              .buildInstr(TargetOpcode::G_ASHR, {NarrowTy},
----------------
Considering efficiency it might be better to create only one G_ASHR that will have sign of "extension point SEXT_INREG" and copy it to remaining registers that hold higher bits.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:770
+                                          {PartialExtensionReg, AshrCstReg})
+                              ->getOperand(0)
+                              .getReg());
----------------
`getOperand(0).getReg()` -> `getReg(0)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61289