[PATCH] D121183: [RISCV] Generate correct ELF EFlags when .ll file has target-abi attribute

[luxufan via Phabricator via llvm-commits]

StephenFan added inline comments.
Herald added a subscriber: arichardson.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:262
+                               ABIName);
+
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
----------------
khchen wrote:
> Why do we need to have the additional checking here? I think we already have check in line 246~256?
> 
> Could we have a test to show this check could catch an error?
Not only the `computeTargetABI` function computes the target abi, but also check if the abi string and feature bits is valid (by calling `parseFeatureBits`). Before this patch, the `computeTargetABI` function was called in the `RISCVAsmBackend` 's constructor. And the `RISCVAsmBackend` was used by almost all the tools(llvm-mc, llc ...) no matter output asm or object file, so the checking of abi string and feature bits are must be did. However, In this patch, I moved the calling of `computeTargetABI` function into `RISCVTargetELFStreamer`. If we use `llvm-mc` and the output file is not object file, the `RISCVTargetELFStreamer` is not needed and will not be constructed. Then the checking of abi string and feature bits will not be did. And some existing test cases that check abi string and feature bits will fail. So I added these extra checking here.

And I also think these extra checking is weird. Do you have a better solution ?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp:89
+
+  // Check if Feature string is valid
+  auto ISAInfo =
----------------
khchen wrote:
> Why do we need to have additional check here?
> Could we have a test to show this check could catch an error?
The reason is the same as what I said above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121183