[llvm] [RISCV][MC] Implement ISA mapping symbols (PR #67541)

Joe Faulls via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 16 08:50:09 PDT 2023


joe-img wrote:

Many thanks for the review @jrtc27, I've updated the patch to include support for .option assembler directive. A few notes on my changes:

- Removed the additional state and introduced the concept of "NewISAString", whereby if it has a value, the ISA string has changed. I'm not super confident in this, but I wasn't very clear on what you meant by "Why can't we just infer this in the ELF streamer rather than introduce this extra state". How can we infer it when the TargetElfStreamer parses the attributes/options?

- It seems wasteful that both the AsmParser and RISCVElfStreamer maintain an options stack, but they actually hold different data. I.E. AsmParser cares about feature bits and ElfStreamer cares about ISA string. So I chose to make heavy use of RISCVISAInfo to translate the features to string just inside ElfStreamer.

- The error handling in emitDirectiveOptionArch seems a little superfluous as this would have been checked by AsmParser first (I think this is the only path available to have emitDirectiveOptionArch called?) - let me know if you think it should be changed.

https://github.com/llvm/llvm-project/pull/67541


More information about the llvm-commits mailing list