This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Compress instructions based on function features
ClosedPublic

Authored by simoncook on Jan 24 2020, 2:49 AM.

Details

Summary

When running under LTO, it is common to not specify the architecture
spec, which is used for setting up the target machine, and instead rely
on features specified in each function to generate the correct
instructions.

This works for the code generator, but the RISC-V backend uses the
AsmPrinter to do instruction compression, which does not see these
features but instead uses a MCSubtargetInfo object to see whether
compression is enabled. Since this is configured based on the
TargetMachine at startup, it will result in compressed instructions not
being emitted when it has not been given the 'c' TargetFeature, but the
function has it.

This changes the RISCVAsmPrinter to re-initialize the STI feature set
based on the current MachineFunction, such that compressed instructions
are now correctly emitted regardless of the method used to enable them.

Diff Detail

Event Timeline

simoncook created this revision.Jan 24 2020, 2:49 AM

Thanks for catching this issue, Simon. Alex, will this be cherry-picked to the branch?

apazos added inline comments.Jan 31 2020, 3:42 PM
llvm/test/CodeGen/RISCV/compress2.ll
6

Maybe a more meaningful name is compress-attribute.ll?

evandro added a subscriber: evandro.Feb 5 2020, 1:27 PM

Copying the the STI isn't quite trivial, and we're only using a small subset of what it provides, in compressInst. Can't this be done more tightly?

compressInst calls RISCVValidateMCOperand, which checks things like STI.getTargetTriple().isArch64Bit(). You said that "under LTO it is common to not specify the architecture". If that includes not properly initializing the STI target triple then those checks will fail in some circumstances. Please check also the status of the other members of the STI, in case the validation ends up using them in the future.

@simoncook Thank you for the patch! It's looking good.

@apazos I've had a brief chat with Alex about this. We do not intend to backport this change to 10.0, especially as LTO is not yet enabled for the RISC-V backend.

Copying the the STI isn't quite trivial, and we're only using a small subset of what it provides, in compressInst. Can't this be done more tightly?

compressInst calls RISCVValidateMCOperand, which checks things like STI.getTargetTriple().isArch64Bit(). You said that "under LTO it is common to not specify the architecture". If that includes not properly initializing the STI target triple then those checks will fail in some circumstances. Please check also the status of the other members of the STI, in case the validation ends up using them in the future.

I think isArch64Bit() will always be correct since we have to specify the triple, but I see your point with other features, I can imagine if we call llc/use LTO with +c and then have a function that enables one of the floating point flags, I'll look into and test that one.

As for testing this, it feels a shame to have to duplicate testcases like I have with compress.ll to cover both cases. Does it make sense to extend the ForceFunctionAttrsPass to add target-features to all functions, in which case we can have just one test case and check both ways of having attributes set? (unless anyone knows any other way we can minimize test duplication?)

As for testing this, it feels a shame to have to duplicate testcases like I have with compress.ll to cover both cases. Does it make sense to extend the ForceFunctionAttrsPass to add target-features to all functions, in which case we can have just one test case and check both ways of having attributes set? (unless anyone knows any other way we can minimize test duplication?)

It may be a bit of a hacky solution, but how about this? You keep one test file, add the attribute group references to the test functions, but don't add the attribute group definition to the source. Then in the RUN lines you cat the test source together with either an empty attribute group (the original tests) or the one with the target features.

It may be a bit of a hacky solution, but how about this? You keep one test file, add the attribute group references to the test functions, but don't add the attribute group definition to the source. Then in the RUN lines you cat the test source together with either an empty attribute group (the original tests) or the one with the target features.

That would work. Looking at other targets tests it looks like NVPTX uses this method to do something similar for adding metadata to tests (e.g. test/CodeGen/NTPTX/nvvm-relfect.ll), so I'll rebase this using this pattern to keep just one test.

simoncook updated this revision to Diff 244385.Feb 13 2020, 4:11 AM

Add floating point test, combine compress tests into single test file.

khchen added a subscriber: khchen.Feb 27 2020, 8:38 AM

I have one query at the moment:

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
160

Most backends seem to override AsmPrinter::runOnFunction entirely (the default implementation calls SetupMachineFunction), and avoid overriding SetupMachineFunction. Is there a particular reason you've chosen one vs the other? I note things like the ARM backend are pulling a SubtargetInfo out of the MachineFunction in their runOnFunction implementations, but I realise we end up needing a MCSubtargetInfo here - I presume that's the main reason?

luismarques accepted this revision.Feb 27 2020, 9:20 AM

LGTM.

llvm/test/CodeGen/RISCV/compress-float.ll
2 ↗(On Diff #244385)

Nitpick: one -> once

34 ↗(On Diff #244385)

You're adding nounwind twice.

This revision is now accepted and ready to land.Feb 27 2020, 9:20 AM
simoncook marked 2 inline comments as done.Feb 28 2020, 2:00 AM
simoncook added inline comments.
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
160

No there isn't (or at least I can't remember there being a good reason), but thanks for pointing out this looks odd, going down the route of overriding runOnFunction as other backends do works as well, so I'm updating this to make this change in runOnFunction to match.

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