qiongsiwu1 added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:54
+ auto TargetCPUName = llvm::StringSwitch<llvm::StringRef>(CPUName)
+ .Case("common", "generic")
+ .Case("440fp", "440")
----------------
jamieschmeiser wrote:
> This seems strange. If the option is "generic", it calls
> getPPCGenericTargetCPU(), but if it is "common", it returns "generic." I
> think you may want to also call getPPCGenericTargetCPU() here. There should
> probably also be an assume where this returns that it didn't return "generic"
> if that is the intended result. Also, there should also be tests for what
> happens when "generic" and "common" are specified.
I agree this `generic` vs `common` behaviour is strange. They way this patch
handles `generic` vs `common` keeps the existing behaviour. In short, before
this patch, `clang` already treats `-mcpu=generic` and `-mcpu=common`
differently.
When `-mcpu=generic` is specified, we [[
https://github.com/llvm/llvm-project/blob/d3c3de63ce8416ab2dee7f784e54b00a2aa8ed85/clang/lib/Driver/ToolChains/CommonArgs.cpp#L420
| do some processing depending ]] on the OS and architecture (effectively
calling `getPPCGenericTargetCPU`). This happens because `generic` is not one of
the cases of the big `StringSwitch`, and we return an empty string from
`ppc::getPPCTargetCPU`.
On the other hand, when `-mcpu=common` is specified, the `StringSwitch` maps
`common` to `generic`, and we simply returns `generic` as the target CPU name
[[
https://github.com/llvm/llvm-project/blob/d3c3de63ce8416ab2dee7f784e54b00a2aa8ed85/clang/lib/Driver/ToolChains/CommonArgs.cpp#L418
| here ]].
**//Is this behaviour what we expect? //**If it is not, I will add a bug fix
(with this patch or with a different patch if a separate one is easier to
review). We only have an [[
https://github.com/llvm/llvm-project/blob/2c69e1d19a2b8bcf27ef1c5a4b5cc0410ed81a52/clang/test/Driver/ppc-cpus.c#L23
| existing test case ]] for `generic`, but not for `common`. I will add one
with the bug fix.
================
Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:77
+ .Default(CPUName);
+ return TargetCPUName.str();
}
----------------
jamieschmeiser wrote:
> Why did you change the type from const char *? Couldn't you use
> CPUName->data() in the default instead? With your change, I think it may
> need to create StringRefs around all of the choices and then just get the
> string from them.
I was worried about the [[
https://github.com/llvm/llvm-project/blob/d3c3de63ce8416ab2dee7f784e54b00a2aa8ed85/llvm/include/llvm/ADT/StringRef.h#L129
| comment ]] on top of `StringRef::data()` which says "Get a pointer to the
start of the string (which may not be null terminated).". I was not sure what
would happen if the data inside `CPUName` was not null terminated (it might be
fine) so I thought it would be safer to use the `StringRef`s directly instead.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139720/new/
https://reviews.llvm.org/D139720
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
