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

[Zakk Chen via Phabricator via llvm-commits]

khchen added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:262
+                               ABIName);
+
     const MCObjectFileInfo *MOFI = Parser.getContext().getObjectFileInfo();
----------------
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?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp:37
 namespace RISCVABI {
+
 ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
----------------
redundant line?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:39
+  setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
+                                          MAB.getTargetOptions().getABIName()));
 }
----------------
Original program flow does an assert checking after init the variable ABI. Does make sense to keep the checking?
If yes, maybe we could move the checking into `setTargetABI` function, then we don't need to check it after `getTargetABI` in line 155.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp:89
+
+  // Check if Feature string is valid
+  auto ISAInfo =
----------------
Why do we need to have additional check here?
Could we have a test to show this check could catch an error?


================
Comment at: llvm/test/CodeGen/RISCV/module-target-abi2.ll:16
 ; DEFAULT-NEXT: fcvt.s.w fa0, a0
 ; DEFAULT-NEXT: ret
 ; RV32IF-ILP32F: # %bb.0:
----------------
StephenFan wrote:
> khchen wrote:
> > The elf attribute becomes correct, but I think the assembly is still incorrect?
> > LLVM can not read the -target-abi from IR and overwrite the MCTargetOptions.ABIName,
> > It's why https://reviews.llvm.org/D102582 would like to report an error if users give the mismatch -target-abi comparing to IR's.
> > 
> Do you mind giving a test case that can reflect the assembly is incorrect ? 
> This patch wants to generate the correct eflags if the target-abi information is in IR instead of being specified by command line.
> Indeed, LLVM can not read the -target-abi IR from and overwrite the MCTargetOptions.ABIName, So I put the ABI info in `RISCVTargetStreamer`, and set it before the end of `AsmPrinter`. Because I think at the end of `AsmPrinter`, the ABI value should have been  determined. And `mips` seems to be too.
Sorry, I'm wrong. 
In the getSubtargetImpl the backend already reads the ABI from IR and ignore the checking if MCTargetOptions.ABIName is empty.

Cool, it's a good solution better then I did before in https://reviews.llvm.org/D72246, Thanks!!!

I think for enabling LTO, for now,  we only need to find a way to encode the module scope march in bitcode file (I had PoC in https://reviews.llvm.org/D106347)





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121183