[PATCH] Cleanup inconsistent MachineVerifier+Print usage

[Daniel Sanders]

> -----Original Message-----
> From: Matthias Braun [mailto:matze at braunis.de]
> Sent: 01 December 2014 22:07
> To: Quentin Colombet
> Cc: Commit Messages and Patches for LLVM; Evan Cheng; hfinkel at anl.gov;
> venkatra at cs.wisc.edu; jholewinski at nvidia.com; t.p.northover at gmail.com;
> richard at xmos.com; Nadav Rotem; Daniel Sanders;
> rsandifo at linux.vnet.ibm.com; thomas.stellard at amd.com
> Subject: Re: [PATCH] Cleanup inconsistent MachineVerifier+Print usage
> 
> Hi Quentin,
> 
> thanks for the review!
> 
> > On Dec 1, 2014, at 1:55 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
> >
> > Hi Matthias,
> >
> > I like the direction of this!
> >
> > Here are a few commets on 0002-CodeGen-Add-print-and-verify-pass-
> after-each-Machine.patch:
> >   if (getARMSubtarget().isThumb2())
> >     addPass(createThumb2ITBlockPass());
> > -
> > -  return true;
> >
> > Shouldn't we have a call to the verifier in the else case?
> >
> >
> > -      addPass(createHexagonCFGOptimizer(TM));
> > -  return false;
> > +      addPass(createHexagonCFGOptimizer(TM), false);
> >
> > Although I like the verbosity of this new approach, shouldn't this call be a
> ..., false, false?
> >
> > There are a couple of other places like this. I think we should decide
> whether or not we want the whole verbosity. I leave that to the related
> target owners.
> > Do not hesitate to ping them!
> I assumed having a few more intermediate steps printed when -print-
> machine-instrs is enabled is a good thing. Anyway CC'ing the target owners to
> hear some more opinions on whether it is a problem/unwanted to print
> machine code between every pass instead of just where it is manually
> requested with printAndVerify().

I agree that more intermediate verification and printing steps is a good thing as long as the option is enabled. Could you enable the print and verify after createMipsDelaySlotFillerPass() and createMipsLongBranchPass() too?

By the way, this seems to be the case before your patch but I was surprised to learn that -print-after-all doesn't print/verify the IR after any of the passes affected by -print-machineinstrs. Is this intentional?

> Greetings
> 	Matthias
> 
> >
> > Thanks,
> > -Quentin
> >
> > On Dec 1, 2014, at 11:12 AM, Matthias Braun <matze at braunis.de> wrote:
> >
> >> Hi Evan, list,
> >>
> >> attached are patches that cleanup some of the mess in
> CodeGen/Passes.cpp; specifically they change the behaviour to add
> print+verify passes after each MachineFunction pass by default instead of on
> request. This should help debugging as you don't miss intermediate steps
> that are not printed and should improve dicipline to produce correct machine
> code as newly written passes will have verification enabled by default.
> >>
> >> Greetings
> >>   Matthias
> >>
> >> <0001-CodeGen-Let-MachineVerifierPass-own-its-banner-
> strin.patch><0002-CodeGen-Add-print-and-verify-pass-after-each-
> Machine.patch><0003-X86-do-not-disable-machine-verification-
> anymore.patch><0004-ARM-do-not-disable-machine-verification-
> anymore.patch><0005-AARCH64-do-not-disable-machine-verification-
> anymore.patch>______________________________________________
> _
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits