Page MenuHomePhabricator

[RISCV][Clang] Compute the default target-abi if it's empty.
ClosedPublic

Authored by khchen on Jul 7 2021, 8:17 AM.

Details

Summary

Every generated IR has corresponding target-abi value, so encode a non-empty value would improve the robustness and correctness than empty one.

Diff Detail

Event Timeline

khchen created this revision.Jul 7 2021, 8:17 AM
khchen requested review of this revision.Jul 7 2021, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 8:17 AM
jrtc27 added a comment.Jul 7 2021, 8:42 AM

Extract the code out into a shared function, don't duplicate it otherwise we'll get confusing inconsistent defaults. Especially when there are proposals to change the defaults as currently there are some weird interactions where specifying seemingly-redundant flags changes the behaviour (e.g. clang -target riscv32-unknown-elf gets you rv32imac/ilp32, but clang -target riscv32-unknown-elf -mabi=ilp32 gets you rv32imafdc/ilp32).

khchen updated this revision to Diff 357148.Jul 8 2021, 12:01 AM

address jrtc27's comment.

asb added a comment.Jul 22 2021, 5:07 AM

This looks like a good improvement to me - anything that still makes it a "[PoC]" proof of concept?

arichardson added inline comments.Jul 22 2021, 5:34 AM
llvm/include/llvm/Support/TargetParser.h
178

Maybe this should be computeDefaultABIFromArch()? If llvm::RISCVISAInfo can be used here passing const llvm::RISCVISAInfo& to the function would also avoid some duplicated lines.

khchen updated this revision to Diff 361216.Jul 23 2021, 8:06 AM

address arichardson's comment, thanks!

khchen marked an inline comment as done.Jul 23 2021, 8:07 AM
khchen retitled this revision from [PoC][RISCV][Clang] Compute the default target-abi if it's empty. to [RISCV][Clang] Compute the default target-abi if it's empty..Jul 23 2021, 8:07 AM
jrtc27 added inline comments.Jul 23 2021, 8:11 AM
clang/lib/Basic/Targets/RISCV.cpp
252

The ISAInfo includes XLen

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
200

Ditto (it's literally on the line above!)

llvm/lib/Support/TargetParser.cpp
336

I'm not personally a fan of having HasD but using ISAInfo.hasExtension("e") in the if. I get why it's done, but I think consistency would be better. Personally I'd just inline the ISAInfo.hasExtension("d"), it's not worth a local variable.

jrtc27 added inline comments.Jul 23 2021, 8:15 AM
clang/lib/Basic/Targets/RISCV.cpp
250

I don't think using the getter makes sense. Currently it's entirely equivalent to just reading ABI so serves little purpose, but in theory there could be additional logic added to it (like asserting it's not empty) that would break this. Given you already assume the name of the field below by assigning to it, just read ABI directly.

khchen updated this revision to Diff 361264.Jul 23 2021, 9:55 AM

address @jrtc27's comment, thanks! I forget RISCVISAInfo include XLen..

jrtc27 added inline comments.Jul 23 2021, 10:47 AM
llvm/lib/Support/TargetParser.cpp
341–344

makes me feel more comfortable inside, and also is a bit more like the old code where we did explicitly check 32 and 64 (though that instead fell back on the next step, rather than asserting).

The commit message needs rewriting to reflect the final patch. Also, "Explicitly target-abi encoded in IR is clear than empty target-abi." is wrong, it's not about clarity, it's about robustness and correctness.

clang/lib/Basic/Targets/RISCV.cpp
249

I don't think this warrants a comment, and the current grammar is a bit dodgy, but if you still want one: "Compute the default ABI based on the target features"

khchen updated this revision to Diff 361568.Jul 25 2021, 6:42 PM
khchen marked 6 inline comments as done.

address @jrtc27's comment, thanks again!

asb accepted this revision.Jul 27 2021, 2:27 AM

This looks good to me, and as Jessica says this patch improves robustness and correctness so it would be great to land it for 13.x.

@jrtc27 - I think @khchen has reflected all of your comments, but please double-check if you get a chance.

This revision is now accepted and ready to land.Jul 27 2021, 2:27 AM

Commit message needs rewriting still, but the patch looks good to me

khchen edited the summary of this revision. (Show Details)Jul 27 2021, 7:08 AM

This patch depends on D105168,
Please help to review it if you have time.
Thanks!

This revision was landed with ongoing or failed builds.Dec 10 2021, 9:22 AM
This revision was automatically updated to reflect the committed changes.