This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Added isCompressibleInst() to estimate size in getInstSizeInBytes()
ClosedPublic

Authored by apazos on Oct 1 2019, 12:42 PM.

Details

Summary

Modified compression emitter tablegen backend to emit isCompressibleInst()
check which in turn is used by getInstSizeInBytes() to better estimate
instruction size. Note the generation of compressed instructions in RISC-V
happens late in the assembler therefore instruction size estimate might be off
if computed before.

Diff Detail

Event Timeline

apazos created this revision.Oct 1 2019, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 12:42 PM
asb added a comment.Oct 3 2019, 5:51 AM

I like the direction of this, Thanks Ana. Did you look at modifying getInstSizeInBytes? That would benefit other callers beyond the outliner.

Hi Ana, I'm yet to try this out on our benchmarks, but it looks like a good implementation to me. Considering Alex's comments above, should this patch be reduced to just the isCompressible check and I can merge the rest into the machine outliner patch after adding this one as a parent?

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
502

Nitpick: this should be called isCompressibleInst (IE with an i rather than an a).

Thanks Alex and Lewis.
I will break the patch in 3 commits:

  1. fix missing ImmLeaf predicate, which I will merge directly (no review).
  2. Compress Instr table gen back end changes. I will post this patch for review.
  3. Modify getInstSizeInBytes to invoke isCompressibleInst (thanks for the name fix!). This one I will run a few more correctness tests, and then post for review.
apazos marked an inline comment as done.Oct 4 2019, 3:46 PM
apazos added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoC.td
141 ↗(On Diff #222675)

just noted a copy/paste error: I used isShiftedUInt, instead of isShiftedInt in both simm9_lsb0 and simm12_lsb0. Will fix it when committing the change.

apazos updated this revision to Diff 223643.Oct 7 2019, 1:52 PM
apazos retitled this revision from [RISCV] WIP better estimate size of outlined block with C extension enabled to [RISCV] Added isCompressibleInst() to estimate size in getInstSizeInBytes().
apazos edited the summary of this revision. (Show Details)
apazos updated this revision to Diff 225098.Oct 15 2019, 12:19 PM

Added check to verify whether MI is parentless or not because isCompressible call depends of function/module info.

Did this get merged, or is it still waiting on reviewers?

apazos updated this revision to Diff 232015.Dec 3 2019, 4:35 PM

Rebased.
With this change, the regressions observed with the machine outliner are eliminated.
Sam, I don't think any one has reviewed yet.

lenary added a reviewer: lewis-revill.
asb accepted this revision.Dec 5 2019, 8:07 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 5 2019, 8:07 AM

Is this waiting for someone with commit access to go ahead?

My commit permissions were fixed last week, I will rebase and merge.

This revision was automatically updated to reflect the committed changes.