Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[PowerPC] Partially enable the ISEL expansion pass.

Authored by jtony on Nov 27 2017, 8:29 AM.



The pass to expand ISEL instructions into if-then-else sequences in patch D23630 is currently disabled. This patch partially enable it by always removing the unnecessary ISELs (all registers used by the ISELs are the same one) and folding the ISELs which have the same input registers into unconditional copies.

Diff Detail


Event Timeline

jtony created this revision.Nov 27 2017, 8:29 AM
jtony removed 1 blocking reviewer(s): power-llvm-team.Nov 27 2017, 8:29 AM
syzaara added inline comments.
268 ↗(On Diff #124389)

Won't these special cases already be handled by expandAndMergeISELs?

syzaara added inline comments.Nov 28 2017, 6:38 AM
279 ↗(On Diff #124389)

It seems that Note 1 is no longer satisfied since you have already folded the ISELs in expandAndMergeISELs.

jtony marked 2 inline comments as done.Nov 28 2017, 6:58 AM
jtony added inline comments.
268 ↗(On Diff #124389)

That's a very good question. I considered about that. The majority of these two special cases should have been handled upstream. But because the way we traverse all the ISELs (non backtracking) in the case we have consecutive ISELs which have the same condition(could be merged together) and if the first one is not these two special cases, we will put all the mergeable ISEL in the list and handle them here (expand all of them in the same if-then-else branches) . Therefore, it is still possible we could come here. e.g:

%r3 = ISEL %r5, %r4, %cr0gt
    %r3 = ISEL %r4, %r4, %cr0gt   <---- this is special cases 2
   %r3 = ISEL %r3, %r3, %cr0gt   <---- this is special cases 1
   %r3 = ISEL %r7, %r8, %cr0gt

But if we disabled ISEL-expansion, we are guaranteed to remove all the special cases upstream in the expandAndMergeISELs (which is our mainly cases, since the expansion is disabled)

279 ↗(On Diff #124389)

Same here.

syzaara added inline comments.Nov 28 2017, 9:08 AM
279 ↗(On Diff #124389)

If we have the case of:
foldable isel
foldable isel
regular isel

where both the foldable isels can be merged, previously with expansion enabled, we would merge them. Now, with the expansion enabled, they will be changed to move registers before. So we won't be getting the same behavior with the expansion enabled, making this comment invalid.

jtony marked 5 inline comments as done.Nov 28 2017, 10:45 AM
jtony added inline comments.
279 ↗(On Diff #124389)

That's right Zaara. We could guard the first two special cases handling inside expandAndMergeISELs() with condition if (!ExpandISELEnabled) if we really want to keep the the behavior in the original implementation. Or If we are OK for the change, we just need to update the comments here. @nemanjai Do you have any comments?

echristo edited edge metadata.Nov 28 2017, 1:47 PM

How are we generating these isels if they're basically nops?

I would prefer to have an MIR test case for both cases so we can test that we do the right thing when we come across these redundant/foldable ISEL's. Furthermore, it would be good to have an IR test case for both cases with comments as to what transformation has lead to the redundant case - so 2 IR and 2 MIR test cases.

204 ↗(On Diff #124389)

I would add a comment here that the non-redundant isel 0, 0, 0, N would not satisfy these conditions as it would be ISEL %R0, %ZERO, %R0, %CRN.

209 ↗(On Diff #124389)

This will erase it from the MBB but not from this data structure that we've got all the ISEL instructions in. Is that safe? Are we guaranteed not to iterate over this thing again and blow up?
I'd personally prefer not to have this dangling pointer to a deleted instruction.

217 ↗(On Diff #124389)

Why do we not use the standard opcode for copying a register since other passes know about that opcode? Namely MR or rather the base opcode OR.

221 ↗(On Diff #124389)

Same comment regarding the dangling pointer to the ISEL instruction.

229 ↗(On Diff #124389)

Add a comment here that this will eat up ISEL instructions without considering whether they may be redundant or foldable to a register copy. So we still keep the handleSpecialCases() function in case we've done this.

279 ↗(On Diff #124389)

I would not try to keep the old semantics. The comment is actually semantically valid. It is true that "if the passed list has multiple mergeable ISEL's, we won't fold any". However, due to the folding that has happened previously, it is less likely that the list will actually contain any foldable ISEL's.

Or am I misunderstanding what this discussion is about?

123 ↗(On Diff #124389)

Please track down what transformation is producing the redundant ISEL and add that note here.

nemanjai requested changes to this revision.Nov 28 2017, 5:01 PM

Requesting changes so this pops up in my review queue when it is updated.

This revision now requires changes to proceed.Nov 28 2017, 5:01 PM
jtony marked 9 inline comments as done.Dec 2 2017, 6:56 AM

How are we generating these isels if they're basically nops?

They are introduced at simple register coalensing stage. For details please see the comments I added in the test cases.

209 ↗(On Diff #124389)

Yes, it is safe. Here the data structure is traversed once only.

279 ↗(On Diff #124389)

You understanding is right. As long as we don't want to keep the old semantics, we are OK here.

jtony updated this revision to Diff 125268.Dec 2 2017, 7:00 AM
jtony edited edge metadata.
jtony marked 4 inline comments as done.

Address comments from Ecric and Nemanja.

nemanjai accepted this revision.Dec 8 2017, 12:50 PM

LGTM. Feel free to address the comments on the commit.

213 ↗(On Diff #125268)

Please add a comment such as

// FIXME: if the CR field used has no other uses, we could eliminate the
// instruction that defines it. This would have to be done manually since
// this pass runs too late to run DCE after it.
224 ↗(On Diff #125268)

Just add a comment here that you're using both the TrueValue and FalseValue operands so as not to lose the kill flag if it is set on either of them.

230 ↗(On Diff #125268)

I think it's a little more clear/readable if you just make this an else if(...) {...} rather than an if nested in the else block.

This revision is now accepted and ready to land.Dec 8 2017, 12:50 PM
jtony marked 3 inline comments as done.Dec 11 2017, 7:28 AM
This revision was automatically updated to reflect the committed changes.