[PATCH] D75533: [ARM][LowOverheadLoops] Handle reductions

[Sam Parker via Phabricator via llvm-commits]

samparker marked 5 inline comments as done.
samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:179
+  struct Reduction {
+    MachineInstr *Init;
+    MachineInstr &Copy;
----------------
SjoerdMeijer wrote:
> Can you comment what these members are? I agree that most are self-explanatory, but I am e.g. interested in `Init`, and if it e.g. can be null (there is a check in the fixup function), and what the meaning is of that.
'Init' is the possible instruction that maybe initialising our result register, such as a mov #0, but we won't necessarily have an instruction doing this.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:630
+  MachineInstr *VPSEL = *LiveOutUsers.begin();
+  if (VPSEL->getOpcode() != ARM::MVE_VPSEL)
+    return false;
----------------
SjoerdMeijer wrote:
> nit: was just curious why we expect the first item in the set to be the vpsel. Can we rely on that with a set?
Because the set only has one member if we get here.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:644
+  // TODO: Support more operations that VADD.
+  switch (VCTP->getOpcode()) {
+  default:
----------------
SjoerdMeijer wrote:
> Could this be a good candidate for a helper function in ARMBaseInstrInfo.h?
I'm not sure how relevant it is to the rest of the backend, but it would be more readable here as a local helper - especially once we add more supported opcodes.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:661
+
+  // Check that the VORR is actually a VMOV.
+  MachineInstr *Copy = RDA.getMIOperand(VPSEL, 2);
----------------
SjoerdMeijer wrote:
> I guess you mean VMOV can be an alias for VORR, which you're checking here?
Yes, I don't think we actually have a MVE VMOV instruction.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:688
+  // is storing the reduction.
+  if (!Preheader)
+    return false;
----------------
SjoerdMeijer wrote:
> Can or should this not be checked much earlier?
Yes, at some point in an unknown patch. I've moved it into one of the legality helpers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75533