[PATCH] D93301: [flang][driver] Add support for `-c` and `-emit-obj`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski marked an inline comment as done.
awarzynski added a comment.

@clementval & @SouraVX - thank you for your comments! I will upload an updated version shortly.



================
Comment at: clang/include/clang/Driver/Options.td:4329
   HelpText<"Generate machine code, but discard output">;
-def emit_obj : Flag<["-"], "emit-obj">,
-  HelpText<"Emit native object files">;
----------------
SouraVX wrote:
> Please correct if I've misunderstood this change ?
> You're removing this option from here(`clang`) and  defining again at line 4631 as a common option to both `clang` `flang` ? 
> +1 to that.
> However this seems out of the purview of this patch. Do you think having this as a separate patch(with specific intent) would be good(For tracking/isolating changes) ?
> Or the least you can do is convey this intent in this patch Summary too.
> I'm happy with either of those :)
> You're removing this option from here(clang) and defining again at line 4631 as a common option to both clang flang ? 
Correct :)

> However this seems out of the purview of this patch.
Not quite. Without this change you will get an error when using `-emit-obj`:
```
bin/flang-new -fc1 -emit-obj file.f90
error: unknown argument: '-emit-obj'
```
That's because it wouldn't be parsed here: 
https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/flang/lib/Frontend/CompilerInvocation.cpp#L166-L167
(note the definition of `includedFlagsBitmask`). 

I do agree that the Summary should be updated, thanks for pointing that out!


================
Comment at: clang/include/clang/Driver/Options.td:4629
 
-} // let Flags = [CC1Option]
+} // let Flags = [CC1Option, NoDriverOption]
+
----------------
clementval wrote:
> Is it intended to modify the SYCL options? 
This is just updating the comment (so it's just an nfc, SYCL options are not affected). 

This closing `}` corresponds to the opening `{` specified here:: https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/clang/include/clang/Driver/Options.td#L4040. So, currently we have this:

```
let Flags = [CC1Option, NoDriverOption] in {
// A lot of code HERE
} // let Flags = [CC1Option]
```

Instead it should be:
```
let Flags = [CC1Option, NoDriverOption] in {
// A lot of code HERE
} // let Flags = [CC1Option, NoDriverOption]
```

I was mislead by this comment while working on this patch, so to me it seemed like a _related_ change :) But it isn't and should be extracted into a separate patch: https://github.com/llvm/llvm-project/commit/7f19712a6a9e65bdc9a9843ea488030bc12f3acc.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:104
       // TODO:
       // case clang::driver::options::OPT_emit_obj:
       // case calng::driver::options::OPT_emit_llvm:
----------------
clementval wrote:
> Should you remove this line since you have the case for it?
Yes, thanks for pointing out!


================
Comment at: flang/test/Flang-Driver/code-gen.f90:15
+
+! CHECK: code-generation is not available yet
----------------
SouraVX wrote:
> Since it's an `error` NOT a string(or similar) generated, I would rather have `ERROR` as a string check. This way it is self-evident for end reader.
> Sort of:
> ```! ERROR: code-generation is not available yet```
> 
Yeah, `ERROR` would be better. I wish that we had a prospect of Clang's `-verify`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93301