Skip to content

Conversation

@IanButterworth
Copy link
Member

As is outlined in https://community.arm.com/developer/tools-software/tools/b/tools-software-ides-blog/posts/compiler-flags-across-architectures-march-mtune-and-mcpu

In short, you cannot support both x86 and Arm with exactly the same compiler flags. You must use -mcpu for Arm and -march for x86.

I also opened up JuliaCI/julia-buildbot#149 to switch arm builds to mcpu, before realizing I needed to also adjust the Make file here

Make.inc Outdated
do_step = yes
endif

ifdef do_step
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be a better way to do the OR logic here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ifdef do_step
ifneq ($(MARCH)$(MCPU),)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. Incorporated

@yuyichao
Copy link
Contributor

I think you can just pass in CFLAGS etc? Different from MARCH it's not like this is going to be used anywhere else.

Also, this isn't very ARM specific (and FWIW the condition doesn't seem to capture aarch64 which is arguably more important) though I think it could make sense to forward native march to mtune automatically. According to that article this should work on both x86 and arm.

@IanButterworth
Copy link
Member Author

The paradigm just seems to be different for ARM, so I thought it best to directly support MCPU?

Also, that article claims

LLVM compilers only support “native” for the -mcpu and -mtune flags. You cannot use -march=native with LLVM-based compilers.

Is that right? If march=native wasn't being passed to LLVM for user code because of that, perhaps there's reason to now add -mcpu=native for user code?

I'm wading into an area I'm not familiar with so apologies if these are flawed suggestions

@IanButterworth
Copy link
Member Author

And on the condition for catching arm & aarch64, you're right.. I need to think of a better catch there

@yuyichao
Copy link
Contributor

Is that right?

Not sure if that's right but this PR doesn't change that it seems.

If march=native wasn't being passed to LLVM for user code because of that, perhaps there's reason to now add -mcpu=native for user code?

All of the flags mentioned here (march, mcpu, mtune), no matter what you passed in, has nothing to do with user code. MARCH is used as default for JULIA_CPU_TARGET but even that has zero effect on the user code.

@IanButterworth
Copy link
Member Author

Not sure if that's right but this PR doesn't change that it seems.

Sorry, I didn't mean to imply it was. That was a tangential question. I think I'm concluding that I don't think I know enough about the internals of the language to venture any further.

I just tweaked the logic to catch *arm* and *aarch64* in the XC_HOST section. Maybe there's a more robust method

@yuyichao
Copy link
Contributor

Sorry, I didn't mean to imply it was. That was a tangential question. I think I'm concluding that I don't think I know enough about the internals of the language to venture any further.

What I meant is that if it is currently used and we didn't get an error then adding -mtune should be enough...

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 28, 2020

Edit: Done
As a reminder to myself, if this goes ahead we'd probably want to update the advice in the Arm build tips documentation to use MCPU=native

Make.inc Outdated
else
ifneq ($(XC_HOST),)
XC_HOST := $(ARCH)$(shell echo $(XC_HOST) | sed "s/[^-]*\(.*\)$$/\1/")
ifneq (,$(filter ,$(findstring arm, $(ARCH)) $(findstring aarch64, $(ARCH))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does filter do without a filter list? Is this the same as strip?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On review I think this wouldn't have worked. I've updated to the OR format you suggested for the other case. Thanks

@ViralBShah ViralBShah added the system:arm ARMv7 and AArch64 label Apr 28, 2020
@JeffBezanson JeffBezanson merged commit c86f906 into JuliaLang:master May 1, 2020
@IanButterworth IanButterworth deleted the mcpu branch March 4, 2022 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

system:arm ARMv7 and AArch64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants