[PATCH] D139720: [clang][PPC] Checking Unknown Values Passed to -mcpu

[Jamie Schmeiser via Phabricator via cfe-commits]

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