topperc wrote:
> I read through the patch again cleanly, and I don't have any problems with
> the PR at this point. The only thing I would note is that in
> `GetClangTargetABI` we're constructing a string which indicates which ISA
> extensions are enabled (that are relevant here) to return a string like
> `lp64f`, which is then used in `SetupTargetOpts` to add the feature flags
> that should be enabled for clang. We are doing the exact same thing in
> `DisassemblerLLVMC::DisassemblerLLVMC`,
>
> ```
> if (triple.isRISCV()) {
> uint32_t arch_flags = arch.GetFlags();
> if (arch_flags & ArchSpec::eRISCV_rvc)
> features_str += "+c,";
> if (arch_flags & ArchSpec::eRISCV_rve)
> features_str += "+e,";
> if ((arch_flags & ArchSpec::eRISCV_float_abi_single) ==
> ArchSpec::eRISCV_float_abi_single)
> features_str += "+f,";
> if ((arch_flags & ArchSpec::eRISCV_float_abi_double) ==
> ArchSpec::eRISCV_float_abi_double)
> features_str += "+f,+d,";
> ```
>
> etc. It's a small bit of duplication, but I expect these will be at risk of
> diverging if done separately. I wonder if our ArchSpec should have a method
> to get the clang feature flags.
That code doesn't look right. Shouldn't it be using eRISCV_float_abi_mask. It's
treat each single and double as bit positions, but they are really encodings in
2-bit field.
```
if ((arch_flags & ArchSpec::eRISCV_float_abi_mask) ==
ArchSpec::eRISCV_float_abi_single)
features_str += "+f,";
if ((arch_flags & ArchSpec::eRISCV_float_abi_mask) ==
ArchSpec::eRISCV_float_abi_double)
features_str += "+f,+d,";
```
https://github.com/llvm/llvm-project/pull/99336
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
