llvm dev - Apr 2009 - [LLVMdev] Patch: MSIL backend global pointers initialization

Anton Korobeynikov wrote:
> Hi, Artur
>
>   
>> I'm working on that backend now, so probably I'll send some
more patches
>> soon. I'd be grateful if you could give me some suggestions how to
add
>> some test for that backend to the test-suite. On Linux the output code
>> could be run on Mono and compared with outputs for other backends but
>> I'm not sure if it's preferable to involve third party apps.
>>     
> Tests usually should be self-contained. So, you can, for example, just
> grep the output for "critical" points, which should always be
there.
>   
Ok, thanks.
> Please refine you patches to be consistent with LLVM's coding style and
> I'll apply them.
>   
I'm sorry, attached again.

So again, one of them makes the global pointers initialization work.
The second one allows executing code with vararg pinvoke functions under 
Mono. It generates separate function declaration for each call signature.

Artur

-------------- next part --------------
A non-text attachment was scrubbed...
Name: varargs.patch
Type: text/x-patch
Size: 5307 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090403/b21f8234/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: globals_init_fix.patch
Type: text/x-patch
Size: 3011 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090403/b21f8234/attachment-0001.bin>

Hi, Artur
> So again, one of them makes the global pointers initialization work.
> The second one allows executing code with vararg pinvoke functions under
> Mono. It generates separate function declaration for each call signature.
Is this mono-only feature? Or pretty "universal" one?

-- 
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University

Hi Anton
>> So again, one of them makes the global pointers initialization work.
>> The second one allows executing code with vararg pinvoke functions
under
>> Mono. It generates separate function declaration for each call
signature.
>>     
> Is this mono-only feature? Or pretty "universal" one?
>   
In fact this is needed only for Mono, but with that change the code 
works with any CLR implementation.

Artur

Hi, Artur
> +        if (isa<ConstantExpr>(I->constant)){
> +          const ConstantExpr *CE >
dyn_cast<ConstantExpr>(I->constant);
> +          printConstantExpr(CE);
> +          ty = CE->getType();
> +        } else {
> +          const Function * F = dyn_cast<Function>(I->constant);
> +          printValueLoad(F);
> +          ty = F->getType();
> +        }
You don't need to check stuff twice here. 
1. Use dyn_cast<ConstantExpr> instead of isa + dyn_cast
2. In the "else" path you need to be sure, that I->constant is
indeed
Function. Use cast, not dyn_cast.

PS: Also: "if (foo){" => "if (foo) {"

-- 
With best regards, Anton Korobeynikov.

Faculty of Mathematics & Mechanics, Saint Petersburg State University.

Hi, Artur

Minor comments:
> +// Comparision for std::lower_bound used in
> MSILWriter::printExternals()
> +static bool CompareInstructions(Instruction *A,Instruction *B)
> +{
Put brace on the same line as function def.
> +      if ( !F->use_empty() ) // Print only if used
> +      {      
Likewise. Plus use "if (foo)" instead of "if ( foo )". All
code around
uses the first variant.
> +        if (F->isVarArg()){ 
Add space before "{"
> +          std::vector<Instruction*> ivec;
> +          Value::use_const_iterator e = I->use_end();
> +          for(Value::use_const_iterator i = I->use_begin(); i!=e;
> ++i){
Likewise. Also, put space before "("
> +            Instruction *instr = 
> +              const_cast<Instruction*>(dynamic_cast<const
> Instruction*>(*i));
Sounds hacky. Why do you need to cast away const? Maybe you just need
use_iterator instead?
> +            if(!instr) continue;
See above.
> +            //We want each signature just once
> +            std::vector<Instruction*>::iterator ins > +          
std::lower_bound(ivec.begin(), ivec.end(), instr,
> +              CompareInstructions);
> +            if(ins!=ivec.end() &&
instr->isSameOperationAs(*ins))
> continue;
Likewise.

-- 
With best regards, Anton Korobeynikov.

Faculty of Mathematics & Mechanics, Saint Petersburg State University.

Hi Anton,

Anton Korobeynikov wrote:
> Minor comments:
>   
Thanks for your comments and your patience, I'll now check the style 
four times before I send anything ;)
>> +            Instruction *instr >> +             
const_cast<Instruction*>(dynamic_cast<const
>> Instruction*>(*i));
>>     
> Sounds hacky. Why do you need to cast away const? Maybe you just need
> use_iterator instead?
>   
Ok, I've fixed this.

I didn't attached the new patch because I found a bug in my code - in 
special cases I get doubled declarations.

If I have two instructions like those:

%1 = tail call i32 (i8*, ...)* @printf(i8* noalias getelementptr ([16 x 
i8]* @.str, i32 0, i32 0), i32 %0) nounwind
%10 = call i32 (i8*, ...)* @printf(i8* noalias getelementptr ([11 x i8]* 
@.str2, i32 0, i32 0), i32 5) nounwind

Instruction::isSameOperationAs() returns false for those two. Is it a 
bug or I misunderstood something?
In any case I wrote my custom isSame() operator for this but I'll 
appreciate any hints.

Thanks!
Artur

Hi Anton,

Anton Korobeynikov wrote:
>> +        if (isa<ConstantExpr>(I->constant)){
>> +          const ConstantExpr *CE >>
dyn_cast<ConstantExpr>(I->constant);
>> +          printConstantExpr(CE);
>> +          ty = CE->getType();
>> +        } else {
>> +          const Function * F =
dyn_cast<Function>(I->constant);
>> +          printValueLoad(F);
>> +          ty = F->getType();
>> +        }
>>     
> You don't need to check stuff twice here.
> 1. Use dyn_cast<ConstantExpr> instead of isa + dyn_cast
> 2. In the "else" path you need to be sure, that I->constant is
indeed
> Function. Use cast, not dyn_cast.
>
> PS: Also: "if (foo){" => "if (foo) {"
>   
OK, I've fixed that.

Thanks,
Artur
-------------- next part --------------
A non-text attachment was scrubbed...
Name: globals.patch
Type: text/x-patch
Size: 2727 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090406/449a64fe/attachment.bin>