Every generated IR has corresponding target-abi value, so encode a non-empty value would improve the robustness and correctness than empty one.
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).
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.
The ISAInfo includes XLen
Ditto (it's literally on the line above!)
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.
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.
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.
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"