[PATCH] D93054: [ARM] Add bank conflict hazarding

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Perhaps some tests are missing: one inlined suggestion is related to AssumeITCMBankConflict, and I was wondering about other cases like Size > 4 and SP relative access?



================
Comment at: llvm/lib/Target/ARM/ARMHazardRecognizer.cpp:164
+ARMBankConflictHazardRecognizer::ARMBankConflictHazardRecognizer(
+    const ScheduleDAG *DAG, int64_t DDN, bool ABC)
+    : ScheduleHazardRecognizer(), MF(DAG->MF), DL(DAG->MF.getDataLayout()),
----------------
Nit: I would prefer a different name than `ABC`, probably same for DDN.


================
Comment at: llvm/lib/Target/ARM/ARMHazardRecognizer.cpp:184
+  auto MO0 = *L0.memoperands().begin();
+  uint64_t Size0 = MO0->getSize();
+  auto BaseVal0 = MO0->getValue();
----------------
nit: we can just do:

  if (MO0->getSize() > 4)
    return NoHazard;

to exit early/earlier, also because we don't seem to use `Size0`.


================
Comment at: llvm/lib/Target/ARM/ARMHazardRecognizer.cpp:225
+        BasePseudoVal0->kind() == BasePseudoVal1->kind() &&
+        BasePseudoVal0->isConstantPool() && AssumeITCMBankConflict)
+      return Hazard;
----------------
Option `AssumeITCMBankConflict` defaults to false. Do have/need a test with this enabled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93054