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

[Jim Grosbach]

On May 11, 2009, at 6:07 PM, Evan Cheng wrote:

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

Looks like GPR is correct to me. There's no special reason to use tGPR  
there.

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