simon_tatham marked 2 inline comments as done.
simon_tatham added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:449
// as well.
- for (std::string Feature : {"vfp2", "vfp3", "vfp4", "fp-armv8", "fullfp16",
- "neon", "crypto", "dotprod", "fp16fml"})
- if (std::find(std::begin(Features), std::end(Features), "+" + Feature)
!= std::end(Features))
- Features.push_back(Args.MakeArgString("-" + Feature));
-
- // Disable the base feature unconditionally, even if it was not
- // explicitly in the features list (e.g. if we had +vfp3, which
- // implies it).
- Features.push_back("-fpregs");
+ for (std::string Feature : {
+ "vfp2", "vfp2sp", "vfp2d16", "vfp2d16sp",
----------------
SjoerdMeijer wrote:
> This function and this code is truly one of the worst I guess.... I mean,
> it's not worse than it was before, but you'd really hope that there was a
> (targetparser) API that gives you a list of FPUs, possibly matching some
> criteria, rather than yet another list of hard coded strings. It gets worse,
> this is repeated in `lib/Support/ARMTargetParser.cpp`. End of rant! ;-)
>
> More on topic: do we need to add +mve.fp here?
It did occur to me that this code could be made a lot simpler if clang had
direct access to the data about the dependency relations between subtarget
features. Then it could replace all of this with a simpler and more robust set
of tests for things like "does the transitive closure of the feature set
include `vfp4d16sp`?" instead of having to list every individual feature that
//might// appear. But currently that dependency web lives in the ARM target's
tablegen, so it isn't guaranteed to be compiled in to the LLVM that clang is
linking against.
================
Comment at: llvm/lib/Support/ARMTargetParser.cpp:178
+
+ {"+fpregs", "-fpregs", FPUVersion::VFPV2, FPURestriction::SP_D16},
+ {"+vfp2", "-vfp2", FPUVersion::VFPV2, FPURestriction::None},
----------------
SjoerdMeijer wrote:
> This looks familiar.... I don't think we can easily simplify this? Or get
> rid of? I guess that's best done in a follow up when we this working again?
> Perhaps the only insignificant simplification is that we don't need the
> +<string> and -<string> because they are the same.
As I say in the comment a few lines up, we need both versions of the string
because this function is returning a vector of `StringRef` with no mechanism to
free anything it allocates, so it has to return pointers to static string
literals.
(I could do some preprocessor fiddling to expand a single feature name into
both strings, but I tend to assume most people have a less strong stomach for
cpp horrors than I do :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62998/new/
https://reviews.llvm.org/D62998
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
