michaelplatings added inline comments.
================
Comment at: clang/lib/Driver/ToolChain.cpp:237-245
+ // Enumerate boolean flags we care about for the purposes of multilib here.
+ // There must be a smarter way to do it but this gets us started.
+ const struct HasFlag {
+ ID Pos, Neg;
+ bool Default;
+ } HasFlagList[] = {
+ {OPT_fexceptions, OPT_fno_exceptions, true},
----------------
phosek wrote:
> Would it be possible to add this information to
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td?
> I think that would be more maintainable.
I thought about that, and it would be nice - it's a shame that this information
gets copy-pasted across the Clang sources - there are already 5 or so places
that do this. However for this change I think that would be too much scope
creep so I've stuck with the status quo.
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:441
+ std::vector<StringRef> &Features, bool ForAS,
+ unsigned *OutFPUKind) {
bool KernelOrKext =
----------------
phosek wrote:
> Can we use `ARM::FPUKind` as a type here?
I made a new change, D146141, that makes this possible. I've rebased this patch
on that change now so this is done.
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:556
// Honor -mfpu=. ClangAs gives preference to -Wa,-mfpu=.
unsigned FPUID = llvm::ARM::FK_INVALID;
const Arg *FPUArg = Args.getLastArg(options::OPT_mfpu_EQ);
----------------
phosek wrote:
> Is this variable used anywhere in this function?
Yes, although previously in this scope only for `FPUID == llvm::ARM::FK_NONE`.
Now we're using it more since we're returning it.
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.h:70
+ std::vector<llvm::StringRef> &Features, bool ForAS,
+ unsigned *OutFPUKind = nullptr);
int getARMSubArchVersionNumber(const llvm::Triple &Triple);
----------------
phosek wrote:
> Using output argument isn't very common in LLVM and even in this particular
> case seems a bit ad-hoc. I'm wondering if we could instead extract this logic
> into a new function, for example `getARMFPUFeatures`?
It's not unusual in this area of the code. Usually the name is `ArgFPUKind`
which I don't find descriptive so I went with `OutFPUKind` to make it clearer
that it's an output of the function. However since the function had return type
`void` I've now simply changed it to return `FPUKind` directly.
I spent a chunk of time looking into whether I could get a separate
FPU-specific function. Finding the FPU is tightly coupled across a lot of code
with finding the architecture and CPU. Although I'm sure the code could be
better structured, the problem is also inherent because the floating point
support is dependent on `-mfpu`, `-march`, `-mcpu`, `--target`, `-mfloat-abi`
and possibly others so figuring everything out requires parsing many options.
In short, I think returning FPUKind is the lesser evil here, even if it does
seem a bit ad-hoc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142933/new/
https://reviews.llvm.org/D142933
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
