[PATCH] D73339: [RISCV] Compress instructions based on function features

[Simon Cook via Phabricator via llvm-commits]

simoncook added a comment.

In D73339#1864275 <https://reviews.llvm.org/D73339#1864275>, @luismarques wrote:

> 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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73339/new/

https://reviews.llvm.org/D73339