[LLVMdev] RAUW in shift-and reassociation during X86 ISel

[Adam Nemet]

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>