Skip to content

Conversation

@jcdavis
Copy link
Contributor

@jcdavis jcdavis commented Jan 5, 2018

The current reassociate call only handles cases such as (A+C1) + C2. This adds cases for (X+C1)+Y and X+(Y+C2), which allows simplifying

      (A+1)+(B+2)
    =>(A+(B+2))+1
    =>((A+B)+2)+1

from which the existing reassociate will pull out the 2 and we end up with (A+B)+3

@tkrodriguez
Copy link
Member

As @gilles-duboscq suggested this could interact with loop invariant reassociation but that I think that's true of any reassociation so I don't think it matters for this change. I"ll push once this passes the gate.

@jcdavis
Copy link
Contributor Author

jcdavis commented Jan 9, 2018

What's the correct way to avoid the eclipse comment formatter rewriting here?
@formatter:on and @formatter:off ?

@dougxc
Copy link
Member

dougxc commented Jan 10, 2018

Yes, for example

// @formatter:off
vm.eval(
Source.newBuilder("implicit.ahoj=42").
name("Fourty two").
mimeType(L1).
build()
);
Object ret = vm.findGlobalSymbol("ahoj").get();
// @formatter:on

Copy link
Member

@gilles-duboscq gilles-duboscq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do this in the reassociate code? If it is useful for constants i guess it's also useful for loop invariants.

we must check this add isn't a loop induction variable, since reassociation can break loop analysis

I'm wondering what particular problem you are trying to avoid there.
In principle rotating the operation shouldn't change the IV nature of the node so maybe we have an issue somewhere else?

More importantly, this transformation might also create more work: i think we should be more careful about the cases where the intermediate node has usages.
If in x + (y + c), y + c has usages, we will end up with (x + y) + c and y + c in the IR. At the best this will lead to a further reassociation and we would only break-even but otherwise we have just introduced one more +.

@thomaswue thomaswue closed this Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants