Page MenuHomePhabricator

[RISCV] Use computeTargetABI from llc as well as clang
ClosedPublic

Authored by khchen on Jan 27 2022, 1:17 AM.

Details

Summary

Clang computes the default ABI if -mabi is empty
and encode it in LLVM IR module flag since D105555.
For correctness, llc need to give the same target-abi
(Options.MCOptions.ABIName) with ABI encoded in IR.
The getSubtargetImpl already has a check for them only if
Options.MCOptions.ABIName is not empty.

In order to get more robustness we could have a check for
explicit ABI, but now we have two different logic to
compute the default ABI.

The front-end ABI is defautl to the ilp32/ilp32e/lp64, and
ilp32d/lp64d when hardware support for extension D.
The backend ABI is default to the ilp32/ilp32e/lp64.

Due to the default target-abi for llc had changed, I update
some tests by specific the target-abi with old default value
to make the expected result unchanged.

Diff Detail

Event Timeline

khchen created this revision.Jan 27 2022, 1:17 AM
khchen requested review of this revision.Jan 27 2022, 1:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 27 2022, 1:17 AM

I think this is the same idea as D118333? Other than being a cleaner way of achieving the same goal. I've not looked to see if there are any functional differences between the two.

I think this is the same idea as D118333? Other than being a cleaner way of achieving the same goal. I've not looked to see if there are any functional differences between the two.

The goal of this patch is making llvm and clang have same way to get default target-abi.

D113959 only changes the llvm part and it still not same with clang.

khchen retitled this revision from [RISCV] Update computeTargetABI implementation. to [RISCV] Use computeTargetABI from llc as well as clang.Feb 6 2022, 8:17 PM
This comment was removed by khchen.
khchen updated this revision to Diff 406842.Feb 8 2022, 8:24 AM

rebase on D119250 to make changes clear.

Thanks for kito's suggestion!

khchen edited the summary of this revision. (Show Details)Feb 8 2022, 8:25 AM
asb added a comment.Feb 17 2022, 6:00 AM

Thanks, I've put this on the agenda for the RISC-V LLVM sync call today. I think this is more attractive than the previous proposal due to unifying logic between llc and Clang. I could see a counter-argument about llc being a low-level tool that should be controlled very explicitly.

jrtc27 added inline comments.Feb 17 2022, 8:20 AM
llvm/test/CodeGen/RISCV/double-mem.ll
2 ↗(On Diff #406842)

For a bunch of these it seems it'd make more sense to just use a hard-float ABI. I think it's worthwhile keeping this as NFC test-wise, but don't know if it makes sense to update the ABIs first and make this patch smaller or land this then go through at a later date so as to not stall this patch.

asb accepted this revision.Feb 24 2022, 3:35 AM

Thanks, I've put this on the agenda for the RISC-V LLVM sync call today. I think this is more attractive than the previous proposal due to unifying logic between llc and Clang. I could see a counter-argument about llc being a low-level tool that should be controlled very explicitly.

I forgot to update on this. Nobody had particularly strong feelings either way, but I think the overall consensus was that this change seemed sensible. Landing this after the LLVM 14 branch also feels like a good time for it. LGTM - thanks.

This revision is now accepted and ready to land.Feb 24 2022, 3:35 AM
This revision was landed with ongoing or failed builds.Feb 24 2022, 10:00 PM
This revision was automatically updated to reflect the committed changes.