[clang] [llvm] [RISCV] Inline Assembly Support for GPR Pairs ('Pr') (PR #112983)

[Sam Elliott via cfe-commits]

================
@@ -2238,6 +2256,17 @@ MVT RISCVTargetLowering::getRegisterTypeForCallingConv(LLVMContext &Context,
   return PartVT;
 }
 
+unsigned
+RISCVTargetLowering::getNumRegisters(LLVMContext &Context, EVT VT,
----------------
lenary wrote:

This is to prevent an assert that is hit in `RegsForValue::AddInlineAsmOperands` without this code.

I think AArch64 is not hitting this because they didn't test very much - the `amocas.q` example has a lot of hard cases (multiple returns, matching input/output operands) which hit a lot of the hard cases in SelectionDAGBuilder. Note I had to fix one place where AArch64 was definitely wrong - the changes to call `getAsmOperandValueType` when there are multiple outputs from asm - AArch64 should have hit this case and didn't.

I've spent a few more hours this evening trying to trace down this assert, and I feel closer, but not quite there. The testcase to use to get where I've got to (with the assert) is `test_Pr_wide_scalar_inout`.

SelectionDAGBuilder.cpp's `getRegistersForValue` seems to be doing the right thing, always calling this with the `VT` and `RegisterVT` with the same values, as far as I can tell. It seems to create the `OpInfo.AssignedRegs` in such a way that the later call to `getNumRegisters` in `RegsForValue::AddInlineAsmOperands` will get the same value as it did when it was created.

Where this seems to go wrong is the matching inputs code in SelectionDAGBuilder.cpp - https://github.com/llvm/llvm-project/blob/8b55162e195783dd27e1c69fb4d97971ef76725b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L10119-L10139 

This seems to have its own logic about creating the `RegsForValue MatchedRegs(...)` based on the surrounding DAG Context, and seems to avoid the info available in `OpInfo` which should have some relevancy, instead this code is directly pulling out similar (but not quite identical) information from the DAG. When `getNumRegisters` is eventually called in `MatchedRegs::AddInlineAsmOperands`, it uses `MVT::i128` rather than `MVT::riscv_i64_pair`, which means it thinks it needs two registers, not just one. I think this `MVT::i128` is coming from `InOperandVal.getValueType()` when MatchedRegs is created.

When this code is compared to `getRegistersForValue`, the two really don't seem to line up, logically - I would expect them both to be doing similar things, but they're not - how the `RegsForValue` object is created in `getRegistersForValue` is reasonably different to this logic. 

git-blame-ing this code, it seems to have been introduced for supporting i128 on SystemZ - presumably also for paired operands - and they do have a `getNumRegisters` override, which returns `1` - https://reviews.llvm.org/D100788 (refactors have hit a good number of lines around the SelectionDAGBuilder.cpp visitInlineAsm code I believe to be at fault, but nothing that's not NFC since that change).

I don't feel good about the matching inputs code in SelectionDAGBuilder - I don't understand it, and I don't think my addition to `getNumRegisters` should be necessary if SelectionDAGBuilder.cpp worked more closely to how `getRegistersForValue` works. The extra confusing thing here is that `getRegistersForValue` might have mutated `OpInfo` when there's a matched input - but returned `std::nullopt`, so maybe the logic for matched inputs needs to just match the tail end of `getRegistersForValue`. I'm not entirely sure.

You're a lot more familiar with SelectionDAG than I am, do you have advice for what I should be doing here? It does seem like more target-independent code needs to be fixed to get rid of this. Maybe that can come later?

https://github.com/llvm/llvm-project/pull/112983