[PATCH] D59215: [ARM][ParallelDSP] Enable multiple uses of loads

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Mainly nits, one question about the tests.



================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:121
+  public:
+    WidenedLoad(SmallVectorImpl<LoadInst*> &Lds, LoadInst *Wide)
+      : NewLd(Wide) {
----------------
nit: now that you've changed MemInstList, perhaps you can use that here.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:307
 
+static bool isProfitableToWiden(Instruction *I) {
+  switch(I->getOpcode()) {
----------------
bikeshedding names: not sure if 'profitable' is the right word here. I.e., we're just checking some properties of a load instruction here, and don't do e.g. some cost modeling which I associate with 'profitable'. But I will leave this entirely up to you.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:310
+  default:
+    break;
+  case Instruction::Load:
----------------
nit: I don't know what exactly the coding style dictates here, but I wouldn't mind if the break was on the same line as the default clause.


================
Comment at: test/CodeGen/ARM/ParallelDSP/remove-duplicate-loads.ll:1
-; RUN: opt -mtriple=thumbv7em -arm-parallel-dsp -verify %s -S -o - | FileCheck %s
+; RUN: opt -mtriple=thumbv7em -arm-parallel-dsp -verify %s -S -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-OPT
+; RUN: llc -mtriple=thumbv7em %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-LLC
----------------
Was wondering if was confusing to have both OPT and LLC tests in one file, also there appears to be only 1 LLC test?


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

https://reviews.llvm.org/D59215