[llvm-commits] [PATCH] Machine code verifier pass

[Jakob Stoklund Olesen]

On 11/05/2009, at 20.43, Evan Cheng wrote:
> +    const MachineBasicBlock *MBB;
> +    BBInfo *MBBInfo;
> +    const MachineInstr *MI;
> +    const MachineOperand *MO;
>
> I find it strange that these are class ivars. Can you change them to
> local variables and parameters?

They provide context information to the report() messages, but you are  
right, it is a bit weird. Functions declaring local variables of the  
same name doesn't help either. I'll change it.

> +void
> +MachineVerifier::calcMaxRegsPassed()
> +{
> +  // First push live-out regs to successors' vregsPassed. Remember
> the MBBs that
> +  // have any vregsPassed.
> +  DenseSet<const MachineBasicBlock*> todo;
> +  for (MachineFunction::const_iterator MFI = MF->begin(), MFE = MF-
>> end();
> +       MFI != MFE; ++MFI) {
> +    const MachineBasicBlock &MBB(*MFI);
> +    BBInfo &MInfo = MBBInfoMap[&MBB];
> +    for (MachineBasicBlock::const_succ_iterator SuI =  
> MBB.succ_begin(),
> +           SuE = MBB.succ_end(); SuI != SuE; ++SuI) {
> +      BBInfo &SInfo = MBBInfoMap[*SuI];
> +      if (SInfo.addPassed(MInfo.regsLiveOut))
> +        todo.insert(*SuI);
> +    }
> +  }
> +
> +  // Iteratively push vregsPassed to successors until convergence.
> +  while (!todo.empty()) {
>
> I don't think the order of iteration of DenseSet is deterministic.

That is true, but it shouldn't matter. The algorithm will converge to  
the same final state regardless of the iteration order.

> +    // If any regs removed, we need to recheck successors
>
> Missing a period at the end of the sentence.

Fixed.

> Have you had a chance running this through some code?

Yes, I am using it with all my Blackfin test cases. I have also tried  
enabling it permanently when running the CodeGen test suite.

CodeGen/ARM reveals that the verifier does not handle PHI instructions  
properly. I will have to rework this! I also get lots of these errors:

*** Bad machine code: Illegal physical register for instruction ***
- function:    f1
- basic block: ID#0
- instruction: %SP<def> = tADDspi %SP, 1
- operand 0:   %SP<def>
SP is not a tGPR register.

This is not a real error - the thumb back-end rewrites these  
instructions at a later stage, I think.


CodeGen/Alpha gets complaints about %R27 being undefined:

*** Bad machine code: Using an undefined physical register ***
- function:    l12_l94_bc_divide_endif_2E_3_2E_ce
- basic block: ID#0
- instruction: %R29<def> = LDAHg  
<ga:l12_l94_bc_divide_endif_2E_3_2E_ce>, %R27, 1, %R28<imp-def>
- operand 2:   %R27


CodeGen/CellSPU has trouble with live intervals:

	%R3<def> = LRr32 %R127<kill>
	LQDr128 %R127, 32, %R1

*** Bad machine code: Using an undefined physical register ***
- function:    main
- basic block: ID#0
- instruction: LQDr128 %R127, 32, %R1
- operand 0:   %R127


CodeGen/IA64:

# Machine code for _ZSt11lower_boundIPKmmET_S2_S2_RKT0_():
Live Ins: r32
Live Outs: r8

entry: 0x182d6b8, LLVM BB @0x1001f90, ID#0:
	ALLOC %r3, 0, 0, 0, 0
	%r3<def> = PSEUDO_ALLOC
	%r8<def> = ADDL_GA %r1, <ga:_ZN9__gnu_cxx16__stl_prime_listE>
	%r8<def> = LD8 %r8<kill>
	%AR_PFS<def,dead> = MOV %r3<kill>
	RET %r8<imp-use,kill>

# End machine code for _ZSt11lower_boundIPKmmET_S2_S2_RKT0_().

*** Bad machine code: Using an undefined physical register ***
- function:    _ZSt11lower_boundIPKmmET_S2_S2_RKT0_
- basic block: ID#0
- instruction: ALLOC %r3, 0, 0, 0, 0
- operand 0:   %r3


CodeGen/Mips:

Live Ins: %GP
	NOREORDER
	CPLOAD %T9

*** Bad machine code: Using an undefined physical register ***
- function:    bar
- basic block: ID#0
- instruction: CPLOAD %T9
- operand 0:   %T9

I think %T9 should be reserved in PIC mode.


Evan, thanks for reviewing. I will implement your suggestions, and I  
will also need do deal with PHI nodes and EH landing pads properly.  
Then I'll submit a new patch.

/jakob