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

[Qiongsi Wu via Phabricator via cfe-commits]

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