This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Refactor the scheduling predicates
ClosedPublic

Authored by evandro on Dec 6 2018, 8:40 AM.

Details

Summary

Refactor the scheduling predicates based on MCInstPredicate. Augment the number of helper predicates used by processor specific predicates.

Diff Detail

Event Timeline

evandro created this revision.Dec 6 2018, 8:40 AM
andreadb added inline comments.
llvm/lib/Target/AArch64/AArch64SchedPredicates.td
17–27

Needed by D55345 too.
Without that definition, patch D55345 will not build because CheckExtUXTW is not defined (see AArch64SchedPredExynos.td:29).

29–33

This is needed by D55345 too.

64–65

Needed by D55345.

100

Same.

135–144

This definition is also needed by your other patch D55345 in AArch64SchedPredExynos.td:25

137

Same.
It is required by D55345 in AArch64SchedPredExynos.td:35

Note that I made this patch a predecessor for D55345.

Note that I made this patch a predecessor for D55345.

Thanks for clarifying it. I missed the phabricator message where it says that this is a parent patch...

I am going to review it now.

evandro updated this revision to Diff 177008.Dec 6 2018, 10:42 AM
andreadb accepted this revision.Dec 6 2018, 12:43 PM

I only have one question. Otherwise, the new predicates LGTM (their logic seems to match what you wrote in the code comments).

llvm/lib/Target/AArch64/AArch64SchedPredicates.td
354

I am not familiar with aarch64. So, apologies if what I am about to ask is silly...

The comment at line 347 suggest that you want to match a MOVI Vd, #0, LSL #0.

My question is: is the check for operand #2 correct? Is that check equivalent to checking for the presence of a LSL #0? If so, then fine.

This is the tablegen'd definition:

bool AArch64InstrInfo::isZeroFPIdiom(const MachineInstr &MI) {
  switch(MI.getOpcode()) {
  case AArch64::MOVIv8b_ns:
  case AArch64::MOVIv16b_ns:
  case AArch64::MOVID:
  case AArch64::MOVIv2d_ns:
    return MI.getOperand(1).getImm() == 0;
  case AArch64::MOVIv4i16:
  case AArch64::MOVIv8i16:
  case AArch64::MOVIv2i32:
  case AArch64::MOVIv4i32:
    return (
      MI.getOperand(1).getImm() == 0
      && MI.getOperand(2).getImm() == 0
    );
  default:
    return false;
  } // end of switch-stmt
}
This revision is now accepted and ready to land.Dec 6 2018, 12:43 PM
evandro marked 2 inline comments as done.Dec 6 2018, 3:48 PM
evandro added inline comments.
llvm/lib/Target/AArch64/AArch64SchedPredicates.td
354

Correct. The 3rd operand is an immediate bit field comprising 2 bits to encode the shift (with LSL being the only possible one) and 6 bits to encode the shift amount (with the only possible values being a multiple of 8). Checking it for zero guarantees LSL by 0.

andreadb added inline comments.Dec 6 2018, 3:52 PM
llvm/lib/Target/AArch64/AArch64SchedPredicates.td
354

Thanks for the explanation. LGTM

This revision was automatically updated to reflect the committed changes.
evandro marked an inline comment as done.