[PATCH] BasicBlock::getFirstNonPHI

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] BasicBlock::getFirstNonPHI

Bugzilla from ghost@cs.msu.su

Hi,
everytime one has to add instruction at the beginning of a basic block, one
has to skip past PHI nodes that are already there. How about adding a new
method to BasicBlock, to get that first non-PHI instruction? So, adding an
instruction will be as simple as:

      new SomeInstruction(............., BB->getFirstNonPHI())

Patch attached. Comments?

- Volodya

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

BasicBlock.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] BasicBlock::getFirstNonPHI

Chris Lattner
On Sun, 4 Jun 2006, Vladimir Prus wrote:
> everytime one has to add instruction at the beginning of a basic block, one
> has to skip past PHI nodes that are already there. How about adding a new
> method to BasicBlock, to get that first non-PHI instruction? So, adding an
> instruction will be as simple as:
>
>      new SomeInstruction(............., BB->getFirstNonPHI())

Sure, sounds good.  A couple requests:

1. Please add a const version that returns a const Instruction* also.
2. There is no need to check for running off the end of the basic block,
you are guaranteed that a block has a terminator, which is not a PHI.

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] BasicBlock::getFirstNonPHI

Nick Lewycky
Chris Lattner wrote:

> On Sun, 4 Jun 2006, Vladimir Prus wrote:
>
>> everytime one has to add instruction at the beginning of a basic
>> block, one
>> has to skip past PHI nodes that are already there. How about adding a new
>> method to BasicBlock, to get that first non-PHI instruction? So,
>> adding an
>> instruction will be as simple as:
>>
>>      new SomeInstruction(............., BB->getFirstNonPHI())
>
> 2. There is no need to check for running off the end of the basic block,
> you are guaranteed that a block has a terminator, which is not a PHI.

Assuming it's a valid BasicBlock. Which, if I understand correctly,
isn't always true in mid-transformation. Perhaps you should turn it into
an assert instead, to minimize surprises?

I can't check if this actually happens with llvm.org seemingly down, but
I do recall a situation like that. Maybe it was the BB list within a
function?

Nick
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] BasicBlock::getFirstNonPHI

Chris Lattner
On Sun, 4 Jun 2006, Nick Lewycky wrote:
>> 2. There is no need to check for running off the end of the basic block,
>> you are guaranteed that a block has a terminator, which is not a PHI.
>
> Assuming it's a valid BasicBlock. Which, if I understand correctly,
> isn't always true in mid-transformation. Perhaps you should turn it into
> an assert instead, to minimize surprises?

It should already assert when it attempts to dereference the end iterator.

> I can't check if this actually happens with llvm.org seemingly down, but
> I do recall a situation like that. Maybe it was the BB list within a
> function?

As you say, this can only happen mid-transformation...

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] BasicBlock::getFirstNonPHI

Bugzilla from ghost@cs.msu.su
In reply to this post by Chris Lattner
Chris Lattner wrote:

> On Sun, 4 Jun 2006, Vladimir Prus wrote:
>> everytime one has to add instruction at the beginning of a basic block,
>> one has to skip past PHI nodes that are already there. How about adding a
>> new method to BasicBlock, to get that first non-PHI instruction? So,
>> adding an instruction will be as simple as:
>>
>>      new SomeInstruction(............., BB->getFirstNonPHI())
>
> Sure, sounds good.  A couple requests:
>
> 1. Please add a const version that returns a const Instruction* also.

I was considering it, but then decided that given that you can't pass 'const
Instruction*' as 'insert before' parameter of any other instruction,
there's no point in adding it.

Maybe, the method should be

  Instruction* getFirstNonPHI() const

? Given that in C++ constness is generally not deep -- i.e. don't apply to
contained objects.

- Volodya






_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Re: [PATCH] BasicBlock::getFirstNonPHI

Chris Lattner
On Mon, 5 Jun 2006, Vladimir Prus wrote:
>> Sure, sounds good.  A couple requests:
>>
>> 1. Please add a const version that returns a const Instruction* also.
>
> I was considering it, but then decided that given that you can't pass 'const
> Instruction*' as 'insert before' parameter of any other instruction,
> there's no point in adding it.

Ok, that makes sense.

> Maybe, the method should be
>
>  Instruction* getFirstNonPHI() const
>
> ? Given that in C++ constness is generally not deep -- i.e. don't apply to
> contained objects.

True, but that would break "logical constness" which is more useful.  :)
I'd prefer either two versions or just the single non-const version.

Thanks,

-Chris

--
http://nondot.org/sabre/
http://llvm.org/
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Re: [PATCH] BasicBlock::getFirstNonPHI

Bugzilla from ghost@cs.msu.su
Chris Lattner wrote:

> On Mon, 5 Jun 2006, Vladimir Prus wrote:
>>> Sure, sounds good.  A couple requests:
>>>
>>> 1. Please add a const version that returns a const Instruction* also.
>>
>> I was considering it, but then decided that given that you can't pass
>> 'const Instruction*' as 'insert before' parameter of any other
>> instruction, there's no point in adding it.
>
> Ok, that makes sense.
>
>> Maybe, the method should be
>>
>>  Instruction* getFirstNonPHI() const
>>
>> ? Given that in C++ constness is generally not deep -- i.e. don't apply
>> to contained objects.
>
> True, but that would break "logical constness" which is more useful.  :)
> I'd prefer either two versions or just the single non-const version.

Okay, I've checked in the version with single non-const method.

Thanks,
Volodya

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev