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

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
67

class*

79

Prefer llvm_unreachable over assert(false)

118

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?

154

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

299

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

475

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

579

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
202

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

474

Please replace E with a meaningful variable name.

495

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
44

Would these functions fit better in X86RegisterInfo?

50

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?

246

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

318

*for

323

STLExtras.h provides a more convenient variant of find:

auto I = find(LegalDstDomains, RD);
333

return is_contained(LegalDstDomains, RD)

375

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

381

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

404

EnclosedInstrs.find(MI) -> I

453

Should this be moved out to a common location?

546

Please add .mir tests for each replacer

675

0.0

lib/Target/X86/X86TargetMachine.cpp
412

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
56

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

97

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

103

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

125

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

136

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?

160

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!

197

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

253

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

306

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

318

I think he meant "to all domains".

405

Please, add a comment explaining this case.

413

Please, add a comment explaining this behavior.

437

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

512

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

523

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

648

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
495

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
44

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.

136

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

160

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?

202

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

246

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

453

can you suggest a location?
is x86InstrInfo suitable?

512

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.

523

same answer as above.

546

WIP

579

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
136

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

160

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