[PATCH] D110527: [llvm-reduce] Add MIR support.

[Markus Lavin via Phabricator via llvm-commits]

markus marked 2 inline comments as done.
markus added inline comments.


================
Comment at: llvm/test/tools/llvm-reduce/mir/instr-reduce.mir:1
+# RUN: llvm-reduce -mir -mtriple=riscv32 --test %python --test-arg %p/instr-reduce.py %s -o %t
+# RUN: cat %t | FileCheck --match-full-lines %s
----------------
aeubanks wrote:
> is it possible to do this via FileCheck like the other tests?
Maybe but others e.g. `llvm/test/tools/llvm-reduce/remove-instructions.ll` do it with custom python as well and I felt that best resembles a realistic use case scenario.


================
Comment at: llvm/tools/llvm-reduce/MyMachineModule.cpp:22
+
+static std::unique_ptr<MachineFunction> cloneMF(MachineFunction *SrcMF) {
+  auto DstMF = std::make_unique<MachineFunction>(
----------------
aeubanks wrote:
> moving this inside `llvm/lib` would be nice
Agree but maybe we can consider that a longer term goal. This implementation seems to be OK for this purpose but not sure it would be for generic use inside `llvm/lib`.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:106
+    TestRunner &Test, int Targets,
+    std::function<void(const std::vector<Chunk> &, MyMachineModule *)>
+        ExtractChunksFromModule);
----------------
aeubanks wrote:
> I'd prefer if this were still a `Module` param for normal passes to avoid the `MyMachineModule` abstraction inside the delta passes.
> Is there a way to overload this with a `Module` and a `MachineFunction` version? e.g. forwarding them to an internal version of `runDeltaPass()` with wrappers to convert from a `MyMachineModule` to `Module`/`MachineFunction`
Agree! It is always a good idea to reduce the amount of diffs and the patch looks much cleaner now.
I ran into some problems with the `function_ref` and overload resolution so I downgrade to a more primitive function pointer and things worked as expected again. I think the problem is related to https://stackoverflow.com/questions/7608741/c11-template-function-that-takes-a-stdfunction-which-depends-of-template-par but I have not looked at it too closely yet.


================
Comment at: llvm/tools/llvm-reduce/llvm-reduce.cpp:79
+                 cl::desc("Set the target triple"));
+cl::opt<bool> EnableMIR("mir", cl::desc("MIR mode"));
 
----------------
aeubanks wrote:
> it'd be nice if we could automatically infer this from the input (like `llc`)
Yeah, but maybe we can save that for later as well.


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

https://reviews.llvm.org/D110527