This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] Support option '-mcpu' on target AVR.
AbandonedPublic

Authored by benshi001 on Dec 5 2022, 3:11 AM.

Details

Summary

The options '-mcpu' and '-mmcu' has the same functionality except
'-mmcu' is used only for target AVR and MSP430. I think newer AVR
projects should use clang common style options instead of older
avr-gcc style.

Currently we keep '-mmcu' for compatibility with avr-gcc,
and '-mcpu' is superior to '-mmcu'.

Diff Detail

Event Timeline

benshi001 created this revision.Dec 5 2022, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 3:11 AM
benshi001 requested review of this revision.Dec 5 2022, 3:11 AM
benshi001 added inline comments.Dec 5 2022, 3:12 AM
clang/test/Driver/avr-mmcu.c
48

These 4 tests are redundant with their previous lines, so they should be removed.

arichardson added inline comments.Dec 5 2022, 3:14 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
387

It looks to me that -mcpu will always take precedence over -mmcu with this code even if mmcu comes later in the command line. Wouldn't it make more sense to treat them as aliases?

benshi001 updated this revision to Diff 480040.Dec 5 2022, 3:30 AM
benshi001 retitled this revision from [clang][driver] Support option '-mcpu' on target AVR to [clang][driver] Make option '-mmcu' as an alias of option '-mcpu'.
benshi001 marked an inline comment as done.
benshi001 added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
387

That's really a good idea. Thanks!

benshi001 marked an inline comment as done.Dec 5 2022, 3:32 AM
benshi001 edited the summary of this revision. (Show Details)Dec 5 2022, 3:57 AM
benshi001 set the repository for this revision to rG LLVM Github Monorepo.
MaskRay requested changes to this revision.Dec 6 2022, 9:27 AM

This makes -mmcu= available for non-AVR targets, which is not right.
Even for AVR, if this is a plain alias, is this really necessary? Usually having more than one aliases for one functionality just create confusion as users don't know which one is the canonical.
I think one choice is for AVR users to migrate -mmcu= (if any) to -mcpu=, if there are a significant number of uses. Making a project buildable with Clang usually requires porting efforts, and this request may not be unreasonable.

This revision now requires changes to proceed.Dec 6 2022, 9:27 AM
benshi001 updated this revision to Diff 480748.Dec 6 2022, 7:42 PM
benshi001 retitled this revision from [clang][driver] Make option '-mmcu' as an alias of option '-mcpu' to [clang][driver] Support option '-mcpu' on target AVR..
benshi001 edited the summary of this revision. (Show Details)
benshi001 updated this revision to Diff 480749.Dec 6 2022, 7:46 PM

This makes -mmcu= available for non-AVR targets, which is not right.
Even for AVR, if this is a plain alias, is this really necessary? Usually having more than one aliases for one functionality just create confusion as users don't know which one is the canonical.
I think one choice is for AVR users to migrate -mmcu= (if any) to -mcpu=, if there are a significant number of uses. Making a project buildable with Clang usually requires porting efforts, and this request may not be unreasonable.

I have changed to

  1. Both '-mcpu' and '-mmcu' are supported for AVR, and '-mmcu' is not available to others;
  2. '-mcpu' is superior to '-mmcu', and '-mcpu' is encouraged for newer clang built AVR projects;
  3. A warning will emit if both are specified.

I think newr AVR projects should use clang common style options instead of older avr-gcc style.

Does GCC prefer -mcpu= as well? If not, raise a PR there? This seems like a decision we should not unilaterally make on llvm-project side: it's certainly something GCC cares about as well.

aykevl added a comment.EditedDec 22 2022, 7:53 AM

I think having -mcpu as an alias for -mmcu on AVR is fine, but I don't think it's very useful to be honest. The -mmcu (or -mcpu) flag is inherently specific to that particular chip anyway so compatibility is not an issue. In fact, having two flags may even introduce noise and confusion.
I wouldn't be opposed to it though if there is demand for it.

Does GCC prefer -mcpu= as well? If not, raise a PR there? This seems like a decision we should not unilaterally make on llvm-project side: it's certainly something GCC cares about as well.

I guess, but avr-gcc is already barely maintained. Most people use an older version (like 5.2.0) because the newer versions produce absolutely terrible code. So I don't think it matters much.

I think having -mcpu as an alias for -mmcu on AVR is fine, but I don't think it's very useful to be honest. The -mmcu (or -mcpu) flag is inherently specific to that particular chip anyway so compatibility is not an issue. In fact, having two flags may even introduce noise and confusion.
I wouldn't be opposed to it though if there is demand for it.

Does GCC prefer -mcpu= as well? If not, raise a PR there? This seems like a decision we should not unilaterally make on llvm-project side: it's certainly something GCC cares about as well.

I guess, but avr-gcc is already barely maintained. Most people use an older version (like 5.2.0) because the newer versions produce absolutely terrible code. So I don't think it matters much.

I see. Thanks.

benshi001 abandoned this revision.Dec 22 2022, 5:34 PM