[PATCH] D35366: [AArch64][Falkor] Avoid HW prefetcher tag collisions (step 2)

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:214
+
+static Optional<LoadInfo> getLoadInfo(const MachineInstr &MI) {
+  int DestRegIdx;
----------------
gberry wrote:
> MatzeB wrote:
> > I generally prefer long instruction list being in AArch64InstrInfo. I think a good guide is asking yourself, what happens if someone adds a new Load/Store instruction: does this code need to be adapted and will the person adding the load/store be able to find this code?
> > 
> > AArch64InstrInfo, also already has various getMemXXX functions, at a first glance it looks like code duplication. Did you check whether this code could use the existing functions or the existing functions could be rewritten using this code?
> I considered this, but some of the info computed by this function is specific to the Falkor HW prefetcher tag implementation, so I figured if I'm going to need a giant switch op opcodes, I might as well compute everything I need to know in one place, even though some of it is a bit redundant.
> 
> To your point about adding instructions in the future: I don't think that applies in this case since the opcodes supported by Falkor are not going to change going forward (unless we add more pseudo load/store instructions I guess, in which case those would hopefully have been expanded by the time this pass runs).
ok, makes sense.


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:650-651
+    for (MachineInstr &MI : *MBB) {
+      Optional<LoadInfo> LInfo;
+      Optional<unsigned> Tag;
+      LInfo = getLoadInfo(MI);
----------------
Could move the declarations to the assignments.


https://reviews.llvm.org/D35366