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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:43
+
+  assert((MI.getOpcode() == TargetOpcode::G_ANYEXT ||
+          MI.getOpcode() == TargetOpcode::G_SEXT ||
----------------
aditya_nandakumar wrote:
> 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).
I can switch it over to that. I just thought it was a shame to check it on both sides of the call.


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

No, several parts of GlobalISel require it

> Also, if not, does MachineVerifier enforce it?

Yes, it reports 'Generic instruction accessing memory must have one mem operand' if it's not the case


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:63
+                           DstReg, PtrReg, MMO);
+    MI.eraseFromParent();
+    return true;
----------------
rtereshin wrote:
> `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?
That makes sense to me. We won't want to go overboard with that kind of thing though, it's enough to check it in a couple combines


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:70
 bool CombinerHelper::tryCombine(MachineInstr &MI) {
   return tryCombineCopy(MI);
 }
----------------
aditya_nandakumar wrote:
> 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.
They'll combine correctly but the legalizer will immediately decompose them again on most targets at the moment so it's just wasted effort slowing the compile-times. This brings up something I've been wondering how to deal with over the last few days. If we continue down the path we're going, I currently don't see how we're going to manage large collections of combines and several targets.

Suppose the Foo and Bar targets supports the following combines:
  tryCombine
    tryCombineGroup1
      tryA
      tryB
      tryC
    tryCombineGroup2
      tryD
      tryE
      tryF
and each calls tryCombine. Now suppose Bar realizes that tryE is doing more harm than good.

We could make tryE check that the target isn't Bar. The consequence of that is that we have a monolithic implementation covering all targets. It means a target specific change can introduce all-target bugs, all targets must carry everyones complexity, every change needs testing (including performance) for every target, etc.

We could make Bar call tryCombineGroup1, tryD, and tryF instead. The consequence of that is that useful changes to tryCombine, and tryCombineGroup2 won't apply to Bar. Everyone will have to review combines and choose to enable them and as such won't benefit from improvements. They also won't suffer from losses either which is a good thing but I would hope that those are less common.

We could split tryCombineGroup2 to get (this is just one example):
  tryCombine
    tryCombineGroup1
      tryA
      tryB
      tryC
    tryCombineGroup2
      tryD
      tryF
    tryCombineGroup3
      tryE
this of course assumes that ordering between E and F doesn't matter. Of course, if we do that enough then we eventually reach:
    tryA
    tryB
    tryC
    tryD
    tryF
    tryE
bringing with it the same problems as the previous option.

The answer I keep returning to at the moment is that even if the majority of our implementation is C++, we need something declarative to organize and control the combines for each target. We don't necessarily need to go to my original intent of re-using the ISel matcher for combines and thereby doing the majority of the matching in tablegenerated code but we do need a declarative way to control whether a combine is enabled/disabled (which is trivial) and the order they're applied (not so trivial).


================
Comment at: lib/Target/AArch64/AArch64PreLegalizerCombiner.cpp:49
+    return Helper.tryCombineExtendingLoads(MI);
+  }
+
----------------
rtereshin wrote:
> `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 
> and CombinerHelper::tryCombine properly tries all of the combines implemented in CombinerHelper as it's supposed to

At the moment, that will look like a good idea but having a single monolithic tryCombine() isn't going to last long as more combines get implemented and more targets use combines. (see above)


Repository:
  rL LLVM

https://reviews.llvm.org/D45543