[PATCH] D102812: [clang] Don't pass multiple backend options if mixing -mimplicit-it and -Wa,-mimplicit-it

Martin Storsjö via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 20 04:06:37 PDT 2021


mstorsjo added a comment.

In D102812#2770516 <https://reviews.llvm.org/D102812#2770516>, @DavidSpickett wrote:

>> If multiple instances of the -arm-implicit-it option is passed to
>> the backend, it errors out.
>
> Does it make sense to fix that side too?

I guess it could, although I didn't look into whether it's trivial or tricky. This particular case was at least easy, as all options are handled at once place in one single function.

> Seems like we have a few options to handle this on clang's side:
>
> 1. Last of `-mimplicit-it` and `-Wa,-mimplicit-it` wins
> 2. If they're the same, accept it, if they mismatch, error
> 3. Take the (last) one that matches the current input type
>
> There is some precedent for number 3 in 1d51c699b9e2ebc5bcfdbe85c74cc871426333d4 <https://reviews.llvm.org/rG1d51c699b9e2ebc5bcfdbe85c74cc871426333d4>. Quoting from the commit msg:
>
>   Now the same rules will apply to -Wa,-march/-mcpu as would
>   if you just passed them to the compiler:
>   * -Wa/-Xassembler options only apply to assembly files.
>   * Architecture derived from mcpu beats any march options.
>   * When there are multiple mcpu or multiple march, the last
>     one wins.
>   * If there is a compiler option and an assembler option of
>     the same type, we prefer the one that fits the input type.
>   * If there is an applicable mcpu option but it is overruled
>     by an march, the cpu value is still used for the "-target-cpu"
>     cc1 option.
>
> I'm not up on all the background to this change so perhaps doing number 3 is going to break existing use cases.

If case 3 would mean that `-mimplicit-it=` is ignored when compiling assembly files, that would indeed break existing use cases. If using the input file type to disambiguate cases when there's conflicts, then that's fine, although I'm not sure if there's need for it really.

In this case, the compiler level option is around for historical reasons, and we're aiming to phase out its use within some time frame (although there's much less urgency with it now if we get this resolved more cleanly), so I'm not sure if it's worth to complicate things for it.

Also, if considering the predecent for how the option behaves from GCC/binutils; there, there's only the -Wa level option, and one could plausibly compile a C file with inline assembly that requires adding implicit IT instructions, so there, the -Wa option needs to be respected even when the original source is C.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2584
   }
+  if (!ImplicitItCompiler.empty() && !ImplicitItAsm.empty()) {
+    // Set both via compiler flag and asm flag; ok if they match.
----------------
DavidSpickett wrote:
> ```
> if (ImplicitItCompiler.size() && ImplicitItAsm.size()) {
> ```
Thanks, that'd be even nicer.


================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:39
 // ALWAYS: "-mllvm" "-arm-implicit-it=always"
+// ALWAYS-NOT: "-arm-implicit-it={{.*}}"
 // NEVER: "-mllvm" "-arm-implicit-it=never"
----------------
DavidSpickett wrote:
> mstorsjo wrote:
> > This pattern wouldn't detct if there's e.g. `-arm-implicit-it=never -arm-implicit-it=always`, but I added test cases that pass `-Wa,-mimplicit-it=always` twice, where this check should be able to verify that we only output it once.
> Could you do:
> ```
> // ALWAYS-NOT: "-mllvm" "-arm-implicit-it=never"
> // ALWAYS: "-mllvm" "-arm-implicit-it=always"
> ```
> 
> Not sure if doing the NOT moves the check forward to beyond where the always would be.
Thanks, I think that could work to further guard against that case.


================
Comment at: clang/test/Driver/arm-target-as-mimplicit-it.s:44
 // THUMB: "-mllvm" "-arm-implicit-it=thumb"
-// NEVER_ALWAYS: "-mllvm" "-arm-implicit-it=never" "-mllvm" "-arm-implicit-it=always"
-// ALWAYS_NEVER: "-mllvm" "-arm-implicit-it=always" "-mllvm" "-arm-implicit-it=never"
+// MISMATCH: error: unsupported argument '{{.*}}' to option '-mimplicit-it'
 // INVALID: error: unsupported argument '-mimplicit-it=foo' to option 'Wa,'
----------------
DavidSpickett wrote:
> Can you do this check without the regex?
> 
> Reading it as is this doesn't look like a very helpful error. I'd expect something like:
> ```
> error: mismatched values for implicit-it "thumb" and "arm"
> ```
> 
Yeah this is mostly me being lazy in the first iteration of the patch (I wrote it late last night and wanted to have something to show before finishing) and reusing existing diagnostic messages - it does print `unsupported argument 'always and never' to option '-mimplicit-it'` in the current form, which is a bit odd indeed.

But I guess if I'd handle both option variants in the same loop, so we can be certain about which one was set last, we could simply define it as last one set wins and we could get rid of the error case altogether.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102812/new/

https://reviews.llvm.org/D102812



More information about the cfe-commits mailing list