llvm dev - Sep 2014 - [LLVMdev] RAUW in shift-and reassociation during X86 ISel

Hi Dan,

I am trying to understand the change you made in
http://llvm.org/viewvc/llvm-project?view=revision&revision=57465

In order to understand the details, I tried to revert the change but the test
case still passes which is not very surprising after 6 years :(.

I am seeing a related new bug that causes a load that was stashed on the
NodeStack and in RecordedNodes during iSel to get deleted because the RAUW
during MatchAddress CSE’s the load.

I am hoping that you’d still remember some of the details of why we need to
modify the DAG like this.  Thanks.

Adam

Earlier in codegen, before address modes are considered, there's a general
preference for shift-then-and over and-then-shift because it leads to and
instructions with smaller immediate fields. However, in the special case of
an expression feeding an address, in an and-then-shift the shift may be
foldable into the address. The patch you mention is in the code where it's
doing address-mode matching and it knows it wants an and-then-shift to
allow the shift to be folded into the address. The and will still have to
be instruction-selected, so it needs to be updated. However, if the and has
uses other than the address, just rewriting the and will cause them to see
the wrong value. Inserting a shift to complete the translation of
shift-then-and to and-then-shift allows these other uses to see the value
they expect.

I guess you can ask a variety of questions about why things are arranged
this way, but the general approach here predates my involvement.

Dan

On Wed, Sep 17, 2014 at 11:00 PM, Adam Nemet <anemet at apple.com> wrote:
> Hi Dan,
>
> I am trying to understand the change you made in
> http://llvm.org/viewvc/llvm-project?view=revision&revision=57465
>
> In order to understand the details, I tried to revert the change but the
> test case still passes which is not very surprising after 6 years :(.
>
> I am seeing a related new bug that causes a load that was stashed on the
> NodeStack and in RecordedNodes during iSel to get deleted because the RAUW
> during MatchAddress CSE’s the load.
>
> I am hoping that you’d still remember some of the details of why we need
> to modify the DAG like this.  Thanks.
>
> Adam
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140922/752f41c0/attachment.html>

On Sep 22, 2014, at 1:31 PM, Dan Gohman <dan433584 at gmail.com> wrote:
> Earlier in codegen, before address modes are considered, there's a
general preference for shift-then-and over and-then-shift because it leads to
and instructions with smaller immediate fields. However, in the special case of
an expression feeding an address, in an and-then-shift the shift may be foldable
into the address. The patch you mention is in the code where it's doing
address-mode matching and it knows it wants an and-then-shift to allow the shift
to be folded into the address. The and will still have to be
instruction-selected, so it needs to be updated. However, if the and has uses
other than the address, just rewriting the and will cause them to see the wrong
value.
What do you mean by “rewriting the and”?  As far as I understand we only morph
the node we’re matching which is not the AND node but something upstream.  The
resulting AND node is newly created and eventually becomes an operand of the
post-isel version of MatchToNode without affecting the other users of the
original AND node.

(Now, I do see that this is something unorthodox.  For a period of time we have
the new node nowhere in the DAG.  The RAUW does tie it into the DAG.)

(As another side note, I have also wondered if the failure in the original PR
(http://llvm.org/bugs/show_bug.cgi?id=2849) had to do with the ISel priority
queue which you had removed later in
http://llvm.org/viewvc/llvm-project?view=revision&revision=58748.)

Many thanks for your input and sorry to bring you back to this code...

Adam
> Inserting a shift to complete the translation of shift-then-and to
and-then-shift allows these other uses to see the value they expect.
> 
> I guess you can ask a variety of questions about why things are arranged
this way, but the general approach here predates my involvement.
> 
> Dan
> 
> On Wed, Sep 17, 2014 at 11:00 PM, Adam Nemet <anemet at apple.com>
wrote:
> Hi Dan,
> 
> I am trying to understand the change you made in
http://llvm.org/viewvc/llvm-project?view=revision&revision=57465
> 
> In order to understand the details, I tried to revert the change but the
test case still passes which is not very surprising after 6 years :(.
> 
> I am seeing a related new bug that causes a load that was stashed on the
NodeStack and in RecordedNodes during iSel to get deleted because the RAUW
during MatchAddress CSE’s the load.
> 
> I am hoping that you’d still remember some of the details of why we need to
modify the DAG like this.  Thanks.
> 
> Adam
> 
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140922/cc40bdf1/attachment.html>