jamieschmeiser added a subscriber: hubert.reinterpretcast.
jamieschmeiser 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")
----------------
qiongsiwu1 wrote:
> 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.
Yeah, I saw that you weren't changing the behaviour; I just suspect that it was
an existing bug...Not sure who to check with about that @nemanjai?
@hubert.reinterpretcast?
================
Comment at: clang/lib/Driver/ToolChains/Arch/PPC.cpp:77
+ .Default(CPUName);
+ return TargetCPUName.str();
}
----------------
qiongsiwu1 wrote:
> 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.
Probably the same sort of thing that would happen with creating a StringRef
since that calls strlen to set the size of the string it is wrapping :-)
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
