- User Since
- Oct 3 2013, 2:16 AM (333 w, 1 d)
Mon, Feb 17
Fri, Feb 14
Still not sure why all the tests are needed here. The first one looks fine, i.e. we test that --fortran-fe=<path to copy of driver binary we just created> calls to that copy. The second one appears to test the default behaviour with no option, but why does it do it via a different name for clang, why would that make a difference? The second uses a different name for both driver and frontend. Why is that test relevant to the change you have made? As far as I can see, the name of the driver binary has no bearing on its behaviour wrt. calling a frontend binary. What am I missing?
I'd still like to see the nits addressed and comments on the tests addressed before approving.
Wed, Feb 5
Tue, Feb 4
A few comments running through that need addressing.
Sep 23 2019
I think the behaviour for missing flang is fine for now, and I think we can improve on it later on.
We ought to codify (if it is not done already) where flang looks for tools to exec, because I think PATH is probably not the only place it could look to (directory of clang binary, other relative paths, other environment variables and commandline options, etc.) The new test will need revising once we get there.
Sep 18 2019
Thanks for the updates. I think this is now looking good and matches the RFC already posted.
Sep 17 2019
Nov 21 2018
Hi @nickdesaulniers - thanks for the clarification. I was suffering from some PEBCAK of my own when I thought the commits were not on master. Thanks for these patches - a great help.
Nov 20 2018
I have run into this too recently so would love to see this patch land. Did you get anywhere with those lld test failures?
Aug 8 2017
Agreed with what Florian says. My understanding of -target is that it is used to set up which flavour of toolchain we are using and using it to set the architecture is a shorthand for saying '-target=arm... -march=armvX'. I also expect it to be somewhat like GCC where the 'arm-...' triples cover ARM/AArch32 architecture in general and not the ARM ISA per se.
Feb 9 2017
Thanks Manoj - this now also LGTM.
This all LGTM. If I can assume I am allow to approve, then I approve.
Feb 3 2017
Aug 18 2016
Hi all - I was puzzling over how to write a new unit test that would catch this error without just copying the structure of the target parser itself. The best I could come up with is to define a for each architecture extension, a set of all architectures that must enable it by default. You could then iterate over all the extensions and for each check all the architectures and test that those in the set have the enum set and those not in the set do not.
Aug 16 2016
Aug 15 2016
These macros are available for setting compiler-provided preprocessing macros like ACLE. We are not using any of the ARMv8.2-A ones because the ACLE has not caught up sufficiently with the architecture to have defined any.
Jul 13 2016
Mar 23 2016
Hi Eric - no problem at all, you practically wrote it for me after all ;-)
My local run with no threads works with this updated patch.
Mar 22 2016
Thanks for the help Eric. I'm just running a new patch now will put the new patch up when I get back in to office tomorrow.
Sorry - not sure what happened there. That looks better for me.
Mar 3 2016
Feb 18 2016
Oct 21 2015
Committed as r250888
Oct 16 2015
Committed as r250546 with the suggested change.
Oct 9 2015
Oct 7 2015
Abandoning as cfe-commits was not on subscribers
Abandoning as I forgot to add cfe-commits.
Adandoning and starting again with cfe-commits as subscriber.
Oct 2 2015
What do you think about this Renato? Although Alexandros has added a LangOption which is strictly speaking to do with CodeGen rather than Lang, I think this is the only pragmatica way to do this without needing re-engineering work to expose the codegen options to this phase in clang.
Oct 1 2015
Sep 28 2015
Sep 25 2015
Sep 24 2015
In the overall approach, whilst this is the pragmatic approach to get the job done, it clearly takes liberties with the definition of LangOpt as this your new flag is most certainly a CodeGenOpt rather than a LangOpt. I'd like to see what the others say as I don't see a better solution.
Sep 21 2015
Yes - I would be interested in knowing the rationale behind the original AArch64 setting of __ARM_FP_FPENV_ROUNDING.
Sep 17 2015
Sep 8 2015
Jul 31 2015
Agree with Renato's comments, apart from one.
Jul 30 2015
Agreed - I can look at the next revision from Alexandros regarding adding MP and Virtualization to this table (I am less fussed about OS)
Thanks for explaining the Krait situation - this now makes sense to me and I agree that it is not ideal.
Alexandros asked if I could look over the table of extensions, but I have some points on the code in general.
Jul 14 2015
Be careful - you are linking to the internal infocentre rather than the public one.
Jul 3 2015
Apologies for the "miss" in the original review gents.
Jul 1 2015
Jun 25 2015
Wont the definition getAllowAtInIdentifier tell you?
Jun 24 2015
Are there any other targets that we should test it on?
Jun 18 2015
Again no test that the case insensitivity also applies to the feature extensions. Other than that, this seems the same as the previous patch about -mcpu and I see no reason not to commit it.
Jun 12 2015
I have one comment about a missing regression test, but otherwise this patch seems fine to me. I understand the previous version broke the ARM bot, so I will let Renato have last say.
Jun 1 2015
My only observation is that you have changed the Krait codepath, but have not added a test for that.
May 8 2015
May 5 2015
Hi Gabor, Joerg
Feb 27 2015
Feb 19 2015
There's a consensus in the Unix world that only two such syntaxes should be
We only accept those two, and not the other two. This is intentional.
I think that having -target=<triple> and --target <triple> only is awkward.
Why have two version of the same option with different rules on whether they
accept = or spaced arguments?
Oct 10 2014