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

[Evan Cheng]

On May 11, 2009, at 1:45 PM, Jakob Stoklund Olesen wrote:

>
> 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.

Thanks.

>
>> +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.

Ok.

>
>> +    // 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.

Hrm. I would think tADDspi should be changed to target GPR instead of  
tGPR since it's supposed to update SP. Jim, is there a reason why  
that's not the case?

How about x86? :-)
>
>
>
>
> 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.

Great. Thanks.

Evan

>
> /jakob
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits