[PATCH] D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:43
+
+  assert((MI.getOpcode() == TargetOpcode::G_ANYEXT ||
+          MI.getOpcode() == TargetOpcode::G_SEXT ||
----------------
It looks like we have a contract-inconsistency here. `CombinerHelper::tryCombineCopy` assumes it could be called on any opcode, and if it's not a COPY, it's expected to just gracefully return and report it didn't change anything.

While the newly added `CombinerHelper::tryCombineExtendingLoads` requires the opcode belonging to a specific subset.

I think it makes sense to be consistent about it and probably not just within the `CombinerHelper`, but among all the derived combiners and maybe even all global isel combiners in general.

+ @aditya_nandakumar


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:54
+    unsigned PtrReg = DefMI->getOperand(1).getReg();
+    MachineMemOperand &MMO = **DefMI->memoperands_begin();
+    DEBUG(dbgs() << ".. Combine MI: " << MI;);
----------------
Is it possible to have a memory operand missing here?
Also, if not, does MachineVerifier enforce it?


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:63
+                           DstReg, PtrReg, MMO);
+    MI.eraseFromParent();
+    return true;
----------------
`getOpcodeDef` is able to look through copies, therefore I'd expect this combiner to match the following sequence:
```
%v1:_(s16) = G_LOAD %ptr(p0), (load 2)
%v2:_(s16) = COPY %v1(s16)
%v3:_(s32) = G_ZEXT %v2(s16)
```
and produce the following output:
```
%v1:_(s16) = G_LOAD %ptr(p0), (load 2)
%v2:_(s16) = COPY %v1(s16)
%v3:_(s32) = G_ZEXTLOAD %ptr(p0), (load 2)
```
Do you think it's a good idea to add tests like this and control that this, in fact, actually happens and happens correctly?


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:70
 bool CombinerHelper::tryCombine(MachineInstr &MI) {
   return tryCombineCopy(MI);
 }
----------------
This is clearly supposed to try all of the combines implemented in the helper:
```
  /// If \p MI is extend that consumes the result of a load, try to combine it.
  /// Returns true if MI changed.
  bool tryCombineExtendingLoads(MachineInstr &MI);

  /// Try to transform \p MI by using all of the above
  /// combine functions. Returns true if changed.
  bool tryCombine(MachineInstr &MI);
};
```


================
Comment at: lib/Target/AArch64/AArch64PreLegalizerCombiner.cpp:49
+    return Helper.tryCombineExtendingLoads(MI);
+  }
+
----------------
`CombinerHelper::tryCombineCopy` contains a bug (it doesn't check that register classes and register banks of its source and destination are compatible ), as soon as it's fixed* **and** `CombinerHelper::tryCombine` properly tries all of the combines implemented in `CombinerHelper` as it's supposed to, this could be replaced with just a single call to `CombinerHelper::tryCombine`, not before, though.

*I'm planning on adding a patch with that fix soon, but it will be dependent on https://reviews.llvm.org/D45732 as the latter makes the former simpler and shorter.

+ @aditya_nandakumar 


Repository:
  rL LLVM

https://reviews.llvm.org/D45543