[PATCH] D61289: [globalisel] Add G_SEXT_INREG

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: llvm/include/llvm/Target/GenericOpcodes.td:40
+// returns true) to allow targets to have some bitwidths legal and others
+// lowered.
+def G_SEXT_INREG : GenericInstruction {
----------------
This comment really needs to do a better job explaining the difference between G_SEXT and G_SEXT_INREG. It only covers the mechanical differences (i.e. that you have an immediate operand), but it says nothing about why this different opcode exists or where it would come from. Is the fact that the IRTranslator never creates such instructions relevant? Should we mention that it is only a legalization artifact? Targets can already say that G_SEXT for certain bitwidths is legal, why don't we just allow them to say which bitwidths should be lowered (instead of adding a new opcode)?


================
Comment at: llvm/include/llvm/Target/GenericOpcodes.td:43
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src, unknown:$sz);
+  let hasSideEffects = 0;
----------------
Nitpick: since you're adding support for imm everywhere else, it would be nice if we could say "imm:$sz" instead of "unknown:$sz" here.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:706
+
+    // So long as the new type is has more bits than the bits we're extending we
+    // don't need to break it apart.
----------------
Typo: is has.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:730
+    // Break it apart. Components below the extension point are unmodified. The
+    // component containing the extension point becomes a narrower SEXT_INREG
+    // Components above it are ashr'd from the component containing the
----------------
Missing period.


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-ext.mir:64
+    ; CHECK-DAG: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
+    ; CHECK-DAG: $w0 = COPY [[DEF]](s32)
     %0:_(s64) = COPY $x0
----------------
Is the order really irrelevant for all of these? If so, maybe commit just the change from CHECK to CHECK-DAG separately. Personally, I wouldn't mind keeping the CHECK lines so we can see what actually changed with this patch. Ditto for the other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61289