llvm dev - May 2016 - SelectionDAGISel::Select's API considered harmful

Can you put something in the release notes when this happens?

Thanks,
Hans

On Sat, May 21, 2016 at 10:57 AM, Justin Bogner via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Update: All in tree backends now implement `void Select`. I'll be
> removing the SelectImpl path on Monday.
>
> Justin Bogner <mail at justinbogner.com> writes:
>> TLDR: Heads up for out of tree backends - you're going to need to
update
>> your *DAGToDAGISel::Select method to unconditionally replace nodes
>> directly instead of returning the desired replacement.
>>
>> So I'm working on fixing the undefined behaviour I described in
>> llvm.org/PR26808. As part of this, we need to stop looking into deleted
>> SDNodes to check if they were, in fact, deleted. A big place that we do
>> this is in SelectionDAGISel::DoInstructionSelection, where we can find
>> this helpful comment that came with the commit that added the check for
>> DELETED_NODE:
>>
>> SelectionDAGISel.cpp says:
>>> // FIXME: This is pretty gross.  'Select' should be changed
to not return
>>> // anything at all and this code should be nuked with a tactical
strike.
>>
>> I'm just gonna go ahead and take this advice.
>>
>> I'll phase this in a couple of steps:
>>
>> 1. Rename Select to SelectImpl in all targets, and implement
"virtual
>>    void Select(SDNode *)" in SelectionDAGISel. I'll move the
current
>>    sketchy behaviour into this version of Select.
>>
>> 2. Update backends one at a time to implement "void Select(SDNode
*)"
>>    instead of SelectImpl.
>>
>> 3. Make SelectionDAGISel::Select pure virtual and remove SelectImpl
>>    entirely.
>>
>> If you have an out of tree backend and you merge from trunk, I
recommend
>> updating in between steps 1 and 3 to avoid breakage after 3 happens.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hans Wennborg <hans at chromium.org> writes:
> Can you put something in the release notes when this happens?
I already updated the release notes in r268693, when I added the void
Select option in the first place :)
> Thanks,
> Hans
>
> On Sat, May 21, 2016 at 10:57 AM, Justin Bogner via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> Update: All in tree backends now implement `void Select`. I'll be
>> removing the SelectImpl path on Monday.
This is done in r270454.
>> Justin Bogner <mail at justinbogner.com> writes:
>>> TLDR: Heads up for out of tree backends - you're going to need
to update
>>> your *DAGToDAGISel::Select method to unconditionally replace nodes
>>> directly instead of returning the desired replacement.
>>>
>>> So I'm working on fixing the undefined behaviour I described in
>>> llvm.org/PR26808. As part of this, we need to stop looking into
deleted
>>> SDNodes to check if they were, in fact, deleted. A big place that
we do
>>> this is in SelectionDAGISel::DoInstructionSelection, where we can
find
>>> this helpful comment that came with the commit that added the check
for
>>> DELETED_NODE:
>>>
>>> SelectionDAGISel.cpp says:
>>>> // FIXME: This is pretty gross.  'Select' should be
changed to not return
>>>> // anything at all and this code should be nuked with a
tactical strike.
>>>
>>> I'm just gonna go ahead and take this advice.
>>>
>>> I'll phase this in a couple of steps:
>>>
>>> 1. Rename Select to SelectImpl in all targets, and implement
"virtual
>>>    void Select(SDNode *)" in SelectionDAGISel. I'll move
the current
>>>    sketchy behaviour into this version of Select.
>>>
>>> 2. Update backends one at a time to implement "void
Select(SDNode *)"
>>>    instead of SelectImpl.
>>>
>>> 3. Make SelectionDAGISel::Select pure virtual and remove SelectImpl
>>>    entirely.
>>>
>>> If you have an out of tree backend and you merge from trunk, I
recommend
>>> updating in between steps 1 and 3 to avoid breakage after 3
happens.
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Mon, May 23, 2016 at 10:01 AM, Justin Bogner <mail at justinbogner.com>
wrote:
> Hans Wennborg <hans at chromium.org> writes:
>> Can you put something in the release notes when this happens?
>
> I already updated the release notes in r268693, when I added the void
> Select option in the first place :)
Perfect, thanks!