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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:43
+
+  assert((MI.getOpcode() == TargetOpcode::G_ANYEXT ||
+          MI.getOpcode() == TargetOpcode::G_SEXT ||
----------------
rtereshin wrote:
> 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
Good catch @rtereshin - I would think being consistent here is nice - ie return gracefully if it's not the opcode we want (unless there's a strong reason to change that).


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:70
 bool CombinerHelper::tryCombine(MachineInstr &MI) {
   return tryCombineCopy(MI);
 }
----------------
rtereshin wrote:
> 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);
> };
> ```
I suspect he's put the calls to tryCombineExtendingLoads in AArch pass as other passes may not handle the extending load opcodes correctly in the legalizer. If/when they're ready, then it makes sense to move them into the generic helper.


Repository:
  rL LLVM

https://reviews.llvm.org/D45543