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

Repository
rL LLVM

Event Timeline

evandro created this revision.Dec 6 2018, 8:40 AM
andreadb added inline comments.
llvm/lib/Target/AArch64/AArch64SchedPredicates.td
17–27 ↗(On Diff #176986)

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

29–33 ↗(On Diff #176986)

This is needed by D55345 too.

64–65 ↗(On Diff #176986)

Needed by D55345.

100 ↗(On Diff #176986)

Same.

135 ↗(On Diff #176986)

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

137 ↗(On Diff #176986)

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 ↗(On Diff #177008)

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 ↗(On Diff #177008)

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 ↗(On Diff #177008)

Thanks for the explanation. LGTM

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