This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by StephenFan on Mar 7 2022, 9:35 PM.

Details

Summary

In the past, when construct RISCVAsmBackend, MCTargetOptions.ABIName would be passed and stored in RISCVAsmBackend.
But MCTargetOptions.ABIName can only be specified by -target-abi xxx in command line, if the .ll file has target-abi attribute, the codegen module will ignore it. And the generated object file would have incorrect EFlags value.

https://github.com/llvm/llvm-project/issues/50591 also caused by this problem.

This patch override the AsmPrinter::emitFunctionEntryLabel function and use it to set the target abi value that get from .ll file's target-abi attribute. And storing the target-abi in RISCVTargetStreamer instead of RISCVAsmBackend.

Diff Detail

Event Timeline

StephenFan created this revision.Mar 7 2022, 9:35 PM
StephenFan requested review of this revision.Mar 7 2022, 9:35 PM

remove the content in patch D121073

khchen added inline comments.Mar 7 2022, 10:48 PM
llvm/test/CodeGen/RISCV/module-target-abi2.ll
16

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.

StephenFan edited the summary of this revision. (Show Details)Mar 7 2022, 10:48 PM
StephenFan added inline comments.Mar 10 2022, 6:31 PM
llvm/test/CodeGen/RISCV/module-target-abi2.ll
16

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.

khchen added inline comments.Mar 11 2022, 8:13 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
262

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?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
37 ↗(On Diff #413708)

redundant line?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
39

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.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
89

Why do we need to have additional check here?
Could we have a test to show this check could catch an error?

llvm/test/CodeGen/RISCV/module-target-abi2.ll
16

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)

StephenFan added inline comments.Mar 15 2022, 8:16 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
262

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 ?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
89

The reason is the same as what I said above.

Address @khchen 's comments.

ChangeLog:

  1. Remove redundent line in RISCVBaseInfo.cpp
  2. Move ABI assert checking into setTarget function.
StephenFan marked an inline comment as done.Mar 15 2022, 8:39 PM
khchen accepted this revision.Mar 16 2022, 10:30 AM

LGTM. thanks!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
89

I think we could remove this checking and update expected ERROR message in mattr-invalid-combination.ll and mattr-invalid-combination.s from standard user-level extension 'e' requires 'rv32' to RV32E can't be enabled for an RV64 target in another NFC patch.

This revision is now accepted and ready to land.Mar 16 2022, 10:30 AM
asb accepted this revision.Mar 17 2022, 6:33 AM

This needs a rebase, but otherwise LGTM. Thanks for addressing this!

rebase and fix warning

StephenFan added inline comments.Mar 22 2022, 11:18 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
89

Hi @khchen, D122290 addresses your suggestion about this.