[PATCH] D39555: Introduce llvm-opt-fuzzer for fuzzing optimization passes

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: lib/FuzzMutate/FuzzerCLI.cpp:48-49
       Args.push_back("-O0");
+    } else if (Opt.startswith("instcombine")) {
+      Args.push_back("-passes=instcombine");
     } else if (Opt.startswith("O")) {
----------------
igor-laevsky wrote:
> bogner wrote:
> > We should add a new/separate handleExecNameEncodedFEOpts or handleExecNameEncodedPassList instead of adding this to BE opts. This fuzzer shouldn't need the backend options AFAIK.
> Yes, I agree. I think best option is to write parametrised version of the `handleExecNameEncodedBEOpts` which would receive the desired mapping between "exec" options and command line options. However I would prefer to do this in a separate change to not clobber this one with the refactoring. 
> Do you think we can leave this code as-is for the initial submission?
If that's what we end up doing this function has the wrong name, since IR passes really aren't BE (ie backend) options. I'd originally been thinking that there wouldn't be any real overlap between the option sets that are allowed here, but I guess the -march handling might be useful for some of the IR passes.

I'd really rather we just make a second function for "pass" options for now on the assumption that we won't really need the complexity of a fully general solution to embedding options in the filename.


================
Comment at: tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp:134-139
+  // Set up target dependant options
+  //
+
+  M->setTargetTriple(TM->getTargetTriple().normalize());
+  M->setDataLayout(TM->createDataLayout());
+  setFunctionAttributes(TM->getTargetCPU(), TM->getTargetFeatureString(), *M);
----------------
igor-laevsky wrote:
> bogner wrote:
> > These don't change from input to input do they? We should probably set this up in the initialize function.
> I don't think we can do this. This is freshly created module, so we need to set correct target parameters during each invocation.
> 
Good point, this bit is fine.


================
Comment at: tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp:141-160
+  // Create pass pipeline
+  //
+
+  PassBuilder PB(TM.get());
+
+  LoopAnalysisManager LAM;
+  FunctionAnalysisManager FAM;
----------------
igor-laevsky wrote:
> bogner wrote:
> > Similarly here - we probably don't need to reconfigure the pass pipeline every time.
> That was what I did initially. However libFuzzer complained about "number of mallocs didn't match number of free's" and disabled "LeakDetection after each iteration". 
> 
> I suspect that somewhere in the pass manager something accumulates some state on each run, maybe to cache analysis results or something in this way. I don't really have good knowledge of this area, but having pass manager initialization on each iteration seems like a bulletproof solution from this kind of problems. 
My main concern here is that we'd like to minimize the work we do here as much as possible so that TestOneInput is fast. This is fine for now and we can maybe try to improve it later.


https://reviews.llvm.org/D39555