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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked 3 inline comments as done.
gberry added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:214
+
+static Optional<LoadInfo> getLoadInfo(const MachineInstr &MI) {
+  int DestRegIdx;
----------------
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).


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:685-687
+  for (auto *MBB : L.getBlocks()) {
+    RS.enterBasicBlockEnd(*MBB);
+    for (auto I = MBB->rbegin(); I != MBB->rend(); RS.backward(), ++I) {
----------------
MatzeB wrote:
> Less `auto` please.
I got rid of the auto on 'MBB', but left it on the iterator since that seems fairly standard, and the variable is only used on the next line (which doesn't use 'auto').


https://reviews.llvm.org/D35366