This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add a pass to convert instruction chains between domains
ClosedPublic

Authored by guyblank on Aug 28 2017, 10:49 PM.

Details

Summary

This pass attempts to find instruction chains in one domain and convert them to equivalent instructions in another domain, if it is found profitable.

The patch adds support only for GPR->Mask conversions.

Diff Detail

Repository
rL LLVM

Event Timeline

guyblank created this revision.Aug 28 2017, 10:49 PM
craig.topper edited edge metadata.Aug 28 2017, 11:23 PM

Some initial comments. I need to look some more.

lib/Target/X86/X86DomainReassignment.cpp
66 ↗(On Diff #113023)

class*

78 ↗(On Diff #113023)

Prefer llvm_unreachable over assert(false)

117 ↗(On Diff #113023)

Could we just pass TII to the methods that use it? If one of the virtual implementation doesn't need it they can ignore it. That way we don't have to store it in the classes?

153 ↗(On Diff #113023)

Same question as above, can we pass TII and MRI to the virtual methods?

298 ↗(On Diff #113023)

I'm not sure static state in a pass is a good practice. If we ever tried to multithread it would be a problem.

474 ↗(On Diff #113023)

Should Worklist be a stack variable here that you can pass by reference to addToWorklist? Does it have lifetime beyond building the closure?

578 ↗(On Diff #113023)

Isn't KMOVB and all other 'B' instructions part of the DQI feature set?

delena added inline comments.Aug 29 2017, 12:24 AM
lib/Target/X86/X86DomainReassignment.cpp
201 ↗(On Diff #113023)

The COPY has only 2 operands, right?
Why do you need so complex computations here?

473 ↗(On Diff #113023)

Please replace E with a meaningful variable name.

494 ↗(On Diff #113023)

I think that you can iterate to MemOp. After the MemOp may be only immediate.

zvi edited edge metadata.Aug 29 2017, 1:32 AM

Please add tests that demonstrate cross-BB transformations.

zvi added a comment.Aug 29 2017, 1:36 AM

Would it be beneficial to add .mir tests?

lsaba added a subscriber: lsaba.Aug 29 2017, 2:04 AM
zvi added inline comments.Aug 29 2017, 3:29 AM
lib/Target/X86/X86DomainReassignment.cpp
43 ↗(On Diff #113023)

Would these functions fit better in X86RegisterInfo?

49 ↗(On Diff #113023)

Can this check be made cheaper and more maintainable by defining a super class for all K reg class and check for membership in the superclass?

245 ↗(On Diff #113023)

How does the delete happen? Are we relying on MachneDCE?

317 ↗(On Diff #113023)

*for

322 ↗(On Diff #113023)

STLExtras.h provides a more convenient variant of find:

auto I = find(LegalDstDomains, RD);
332 ↗(On Diff #113023)

return is_contained(LegalDstDomains, RD)

374 ↗(On Diff #113023)

If the class declaration and definition and in the same unit, make the definition inline in class?

380 ↗(On Diff #113023)

This methods does more than possibly adding to the worklist. Maybe visitRegister would be a better name?

403 ↗(On Diff #113023)

EnclosedInstrs.find(MI) -> I

452 ↗(On Diff #113023)

Should this be moved out to a common location?

545 ↗(On Diff #113023)

Please add .mir tests for each replacer

674 ↗(On Diff #113023)

0.0

lib/Target/X86/X86TargetMachine.cpp
410 ↗(On Diff #113023)

identation

guyblank updated this revision to Diff 113072.Aug 29 2017, 5:42 AM

added a test (gpr-to-mask.ll) to show some cross BB issues

aaboud added inline comments.Aug 29 2017, 7:54 AM
lib/Target/X86/X86DomainReassignment.cpp
55 ↗(On Diff #113023)

Please, pick an order and stick to it. "RC == &X86::VK64RegClass" should be moved line 52.

96 ↗(On Diff #113023)

Would it be better to rename this function as: "getExtraCost" or "getIncreasedCost"?

102 ↗(On Diff #113023)

I think you do not need the "virtual" keyword here and in all places below. The "override" keyword is enough.

124 ↗(On Diff #113023)

To make the comment more clear:
"unless it's dead." -> "unless that register is dead."

135 ↗(On Diff #113023)

Are we sure we do not need to add any implicit operand?
in the "isLegal" function you allowed implicit operands that are not dead, when the DstOpcode implicitly define that physical register, so in such case we need to add that implicit operand to the new instruction, do not we?

159 ↗(On Diff #113023)

Why do not we need to check that MI opcode and DstOpode have same operand types/semantic?
Otherwise, the loop in line 169 will fail!

196 ↗(On Diff #113023)

Should not we return false if (MI->getOpcode() != TargetOpcode:COPY)?

252 ↗(On Diff #113023)

Can you explain this KetTy? what are the "int" and the "unsigned" pair represents?
Please, add a comment.

305 ↗(On Diff #113023)

How about passing the vector by reference? i.e., "&LegalDstDomins".

317 ↗(On Diff #113023)

I think he meant "to all domains".

404 ↗(On Diff #113023)

Please, add a comment explaining this case.

412 ↗(On Diff #113023)

Please, add a comment explaining this behavior.

436 ↗(On Diff #113023)

I do not understand this code, can you add a comment explaining the logic behind this code?

511 ↗(On Diff #113023)

Does that make any sense to continue after we clear all Legal Dst Domains? Should not we just return?

522 ↗(On Diff #113023)

Same question here, why do not we just return!?

647 ↗(On Diff #113023)

If you clear these two containers for each function processing, then why they need to be static?
How about let this function creates them and pass them to all "Closure"s by reference? Can do that at line 665 below.

craig.topper added inline comments.Aug 29 2017, 3:29 PM
lib/Target/X86/X86DomainReassignment.cpp
494 ↗(On Diff #113023)

There are definitely instructions that have a register after the memory op. For example vpblendvb

guyblank marked 28 inline comments as done.Sep 3 2017, 9:40 AM

Thank you all for the comments.

lib/Target/X86/X86DomainReassignment.cpp
43 ↗(On Diff #113023)

i'm not sure, there are all the other register classes (GR_ABCD, and friends) which I did not include here. So putting this isGPR function, as is, in X86RegisterInfo would be a bit misleading I think.

135 ↗(On Diff #113023)

BuildMI will add the implicit operands from the instruction description, which are the ones allowed to be not dead by "isLegal"

159 ↗(On Diff #113023)

I think the responsibility for semantic equivalence is on users of this class (and all other converters).

But there is a hidden assumption here that the functions will be called only with the MI they were created with, when initializing the converters.
Does adding the source op code as a member of the converter and checking/asserting on it sound reasonable?

201 ↗(On Diff #113023)

I'm not sure I understand why you say these are complex computations, can you suggest a simpler sequence?

245 ↗(On Diff #113023)

No, this pass will delete the instruction.
ConvertInstr should returns true when the MI needs to be deleted.

452 ↗(On Diff #113023)

can you suggest a location?
is x86InstrInfo suitable?

511 ↗(On Diff #113023)

I don't think it matters much.
If we return here, we'll get to the other uses eventually, as part of another closure, and mark them as illegal (since they will try to add a register which is part of another closure).
So we might as well get to them now.

522 ↗(On Diff #113023)

same answer as above.

545 ↗(On Diff #113023)

WIP

578 ↗(On Diff #113023)

ooops
Thanks!

guyblank updated this revision to Diff 113704.Sep 3 2017, 9:42 AM

addressed comments

guyblank updated this revision to Diff 115657.Sep 18 2017, 8:07 AM

Added mir test

aaboud added inline comments.Sep 23 2017, 12:17 PM
lib/Target/X86/X86DomainReassignment.cpp
135 ↗(On Diff #113023)

Then please add a comment saying that implicit operands are added/handled by BuildMI, and we only need to add the explicit operands.

159 ↗(On Diff #113023)

I think it is the best option we have.
Add the source opcode and make sure that isLegal will accept only MI with same source opcode, otherwise it will fail.
you have two options here:

  1. actually return false if the MI opcode and the source opcode are not the same.
  2. Only assert on that, but always return true. (this is acceptable, if you think that such case is an implementation issue, rather than user input issue).

In addition, you should add an assertion in the ConverInstr(), that the MI is legal:
assert(isLegal(MI) && "Illegal instruction!");

guyblank updated this revision to Diff 117307.Oct 2 2017, 1:04 AM

Address comments by @aaboud and some other offline comments.

zvi accepted this revision.Oct 16 2017, 1:16 PM

LGTM

This revision is now accepted and ready to land.Oct 16 2017, 1:16 PM
This revision was automatically updated to reflect the committed changes.
anna added a subscriber: anna.Oct 24 2023, 9:40 AM

Hi

It looks like this pass has extreme compile time growth. On icelake machines (where we have AVX-512 enabled), we see llc taking about 18 seconds on an IR and this pass contributes about 17 seconds.

There were also prior reproducers/issues added here: https://github.com/llvm/llvm-project/issues/41517

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 24 2023, 9:40 AM
Herald added a subscriber: pengfei. · View Herald Transcript