[PATCH] D76786: [ARM][MVE] Add support for incrementing gathers
Dave Green via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 4 05:50:07 PDT 2020
dmgreen added inline comments.
================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:42
#include "llvm/Transforms/Utils/Local.h"
+#include <experimental/optional>
#include <algorithm>
----------------
You don't need experimental/optional (we technically need llvm/ADT/Optional.h I think, as we are using the one in llvm. It is likely included transitively at the moment).
================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:512
+ PHINode *Phi = dyn_cast<PHINode>(Offsets);
+ if (Phi == nullptr || Phi->getNumIncomingValues() != 2)
+ // No phi means no IV to write back to; if there is a phi, we expect it
----------------
Maybe add a check that Phi->getBB == L->getHeader too.
================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:517
+ unsigned IncrementIndex =
+ Phi->getIncomingBlock(0) == LI->getLoopFor(I->getParent())->getLoopLatch()
+ ? 0
----------------
Creating a variable for L sounds useful.
================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:526
+ return nullptr;
+ Value *OffsetsIncoming = Add.first;
+ int64_t Immediate = Add.second;
----------------
I think we have to check that OffsetsIncoming == Phi. To make sure it's the loop structure / IV we want it to be.
================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:553
+ &Phi->getIncomingBlock(1 - IncrementIndex)->back());
+ Phi->setIncomingValue(1 - IncrementIndex, OffsetsIncoming);
+
----------------
As we are modifying the Phi inplace, and it can produce different values, I think we unfortunately need to check it has no other uses.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76786/new/
https://reviews.llvm.org/D76786
More information about the llvm-commits
mailing list