Page MenuHomePhabricator

[ARM64EC][clang-cl] Add arm64EC test; NFC
ClosedPublic

Authored by bcl5980 on Sep 27 2022, 10:59 PM.

Diff Detail

Event Timeline

bcl5980 created this revision.Sep 27 2022, 10:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 10:59 PM
bcl5980 requested review of this revision.Sep 27 2022, 10:59 PM
efriedma edited reviewers, added: efriedma; removed: eli.friedman.Sep 28 2022, 11:17 AM
efriedma set the repository for this revision to rG LLVM Github Monorepo.
efriedma added a project: Restricted Project.
efriedma added subscribers: mstorsjo, DavidSpickett.

Please add a testcase verifying the interaction between "/arm64EC" and explicitly specifying "--target". (If we end up ignoring the "/arm64EC" flag, we probably want a warning.)

bcl5980 updated this revision to Diff 463744.Sep 28 2022, 7:16 PM

add warning when /arm64EC has been override

bcl5980 updated this revision to Diff 463746.Sep 28 2022, 7:37 PM

Has Microsoft documented this option? Not doubting it exists but I mostly see how to use it rather than a specific page describing the option and its behaviour.

Not a big deal but if there is one, please include a link to it in the commit message.

clang/lib/Driver/Driver.cpp
1384 ↗(On Diff #463746)

This would read better to me if you put them all in one and put the most important check first like:

if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
    ( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
      TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)) {
clang/test/Driver/cl-options.c
786

Add an ARM64EC-NOT check for this part to check there is no warning.

Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.

789

A space after the ':' is a bit easier to parse IMO (unless this is matching an msvc warning in which case fair enough keep it as is).

bcl5980 updated this revision to Diff 464128.Sep 29 2022, 7:16 PM

address comments.

Has Microsoft documented this option? Not doubting it exists but I mostly see how to use it rather than a specific page describing the option and its behaviour.

Not a big deal but if there is one, please include a link to it in the commit message.

For now, I haven't found any document for this option. I can update later when Microsoft publish the information.

DavidSpickett accepted this revision.Sep 30 2022, 1:59 AM

Please mark comments as done if you feel that they have been addressed. (I know this can be a bit weird, do you mark them or do I, I go with the former, I can disagree if needed)

Lack of docs right now is fine, we have enough to know this exists and it's quite simple.

With my one final comment addressed, this LGTM.

clang/lib/Driver/Driver.cpp
1384 ↗(On Diff #463746)

This is good but I didn't emphasise one bit. Putting the arm64ec option check first saves reading the rest if the reader knows it's not relevant.

This revision is now accepted and ready to land.Sep 30 2022, 1:59 AM
bcl5980 added inline comments.Oct 3 2022, 1:15 AM
clang/lib/Driver/Driver.cpp
1384 ↗(On Diff #463746)

I believe the condition UArgs->hasArg(options::OPT__SLASH_arm64EC) should be much heavier than
(TC.getTriple().getArch() != llvm::Triple::aarch64 || TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec).
So I put the Arch and SubArch check first here. I don't understand why we should put the hasArg check first.

This looks fine as is, all comments addressed as far as I can see.

clang/lib/Driver/Driver.cpp
1384 ↗(On Diff #463746)

If the cost of the check is higher then this is fine keep it as is.

The reason that I personally would reorder them (if costs were equal) is this.

Imagine this didn't have 3 conditions it had 100.

if ((A or B or C or D or E...
     <this continues....>
    ) && some_option_used)

Why did I, the reader, have to parse 99 seemingly random conditions (which have to live in my short term memory) only to find that oh, this is all predicated on that one option being passed. If I don't actually care about that one option, I just wasted my time.

And ok, the average if doesn't have 100 conditions but if you keep this in mind even for smaller checks it adds up over time.

Code is going to be read more than it is written.

This applies to early returns too. Imagine I care about the !=10 case:

void foo(int i) {
  if (i == 10) {
    // 500 lines of code you don't care about 50% of the time
  } else {
    // what you were actually looking for
    return;
  }

That situation does happen a lot, and the same principle can be applied.

void foo(int i) {
  if (i != 10)
     return;

  // 500 lines of code you can read if you actually need to
}
bcl5980 marked 4 inline comments as done.Oct 3 2022, 6:19 AM

Thanks for the detail explaination. I agree that if the cost is the same hasArg is better to move to the first.

This revision was automatically updated to reflect the committed changes.

Missing testcase for the case where the warning is triggered?

Missing testcase for the case where the warning is triggered?

Based on David's comment I remove the test case with specified target.

Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.

If you think that's necessary I can commit a NFC change to add it.

I think we want to test both that the warning isn't triggered when --target isn't specified, and that the warning is triggered when --target is specified.

clang/test/Driver/cl-options.c
787

I think the ARM64EC-NOT might be in the wrong place? I think the warning would show up before the final command line, not after it.

bcl5980 reopened this revision.Oct 4 2022, 1:13 AM
bcl5980 updated this revision to Diff 464909.
bcl5980 retitled this revision from [ARM64EC][clang-cl] Add /arm64EC flag to [ARM64EC][clang-cl] Add arm64EC test; NFC.
This revision is now accepted and ready to land.Oct 4 2022, 1:13 AM

This (and it's followup?) has been landed, right? Please close the revision if so.

bcl5980 closed this revision.Oct 7 2022, 11:45 PM