[PATCH] D61289: [globalisel] Add G_SEXT_INREG

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 13 inline comments as done.
dsanders added a comment.

In D61289#1483719 <https://reviews.llvm.org/D61289#1483719>, @arsenm wrote:

> Is the intention to now use this instead of the current system of relying on the cleanup of legalization artifacts?


This changes the output of the legalization artifact pass but doesn't change the system itself. If a target can make use of the G_SEXT_INREG then it can mark it legal but if it can't then it can lower it and end up in the same place as before (except that the G_CONSTANT is positioned slightly differently).



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:119
 
 class SrcOp {
   union {
----------------
arsenm wrote:
> I think the SrcOp changes should be split to a separate patch
Done, it's D61321


================
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 {
----------------
rovka wrote:
> 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)?
> 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.

Ok I can add to that

> Is the fact that the IRTranslator never creates such instructions relevant?

No, who creates it is irrelevant to the operation of the instruction. There's no guarantee that the legalizer won't receive them as input.

In the case of the IRTranslator, the IRTranslator could create it if it wanted but it's a simple 1:1 converter (for the most part) and chooses not to at the moment as there's no LLVM-IR equivalent. Target-specific passes are also free to create them.

> Should we mention that it is only a legalization artifact?

It's (currently only) created by code that deals with legalization artifacts but it's not a legalization artifact itself.

> 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)?

It becomes important when you start optimizing with GlobalISel. Suppose that ARM's SXTB instruction has a latency of 1 and and LSL/ASR have a latency of 2 and that this includes forwarding paths in the hardware (if any). Having the signextend as a single atom in the MIR becomes useful for emitting the most efficient code since given code like:
  int foo(char a) {
    return (int)a << 2;
  }
it's cheaper to emit:
  sxtb r0, r1
  lsl r0, r0, #2
  // 3 cycles
than:
  lsl r0, r1, #16
  asr r0, r0, #16
  lsl r0, r0, #2
  // 6 cycles
even if you can exploit known-bits to emit:
  lsl r0, r1, #16
  asr r0, r0, #14
  // 4 cycles
it would still be better to use the sxtb. The latter example also illustrates that optimization can make it hard to recognise sign-extension. It gets harder if you also reduce the strength of instructions (maybe lsl r0, r0, #1 is faster as add r0, r0, r0) and there's plenty of ways to make things even more difficult. Essentially, the more mangling the optimizer does while ignorant of the desirable code, the harder it is to select the optimal code at the end.


================
Comment at: llvm/include/llvm/Target/GenericOpcodes.td:43
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src, unknown:$sz);
+  let hasSideEffects = 0;
----------------
rovka wrote:
> 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.
I agree. I've added an untyped_imm which for all functional purposes w.r.t the type constraint system is the same as unknown but at least documents that we expect an immediate.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1708-1709
+  case TargetOpcode::G_SEXT_INREG: {
+    if (!MI.getOperand(2).isImm())
+      return UnableToLegalize;
+    int64_t SizeInBits = MI.getOperand(2).getImm();
----------------
arsenm wrote:
> This should be illegal and caught by the verifier? This shouldn't need to check for illegal cases
I've turned it into an assert rather than remove it since it's easier to debug if the debugger stops where it happens and this is the first use of a real immediate in GlobalISel so the chances of misuse are higher than normal.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1325
+      report("G_SEXT_INREG size must be >= 1", MI);
+    if (Imm >= SrcTy.getSizeInBits())
+      report("G_SEXT_INREG size must be less than source bit width", MI);
----------------
I just spotted this one too. This should be getScalarSizeInBits()


================
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
----------------
rovka wrote:
> 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.
The legalizer doesn't provide any guarantees on the order beyond that defs will precede uses. By changing to CHECK-DAG we make the test robust against future changes to the legalizer too. For this patch, the only thing that changed in many cases was the placement of the G_CONSTANT used in the sign-extending shifts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61289