Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[Arm64] Set Instruction set flags#15798

Merged
CarolEidt merged 1 commit into
dotnet:masterfrom
sdmaclea:PR-ARM64-Set-Instruction-Set-Flags
Jan 19, 2018
Merged

[Arm64] Set Instruction set flags#15798
CarolEidt merged 1 commit into
dotnet:masterfrom
sdmaclea:PR-ARM64-Set-Instruction-Set-Flags

Conversation

@sdmaclea
Copy link
Copy Markdown

@sdmaclea sdmaclea commented Jan 9, 2018

@CarolEidt @RussKeldorph @dotnet/jit-contrib @dotnet/arm64-contrib PTAL

@sdmaclea
Copy link
Copy Markdown
Author

test Tizen armel Cross Checked Innerloop Build and Test

Comment thread src/jit/compiler.cpp
if (jitFlags.IsSet(JitFlags::flag)) \
opts.setSupportedISA(InstructionSet_##isa);
#include "hwintrinsiclistArm64.h"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This sequence is quite confusing. Wouldn't it be clearer just to start with InstructionSet_Base as the default?
e.g.:

// There is no JitFlag to specify the base instruction set; we start with that as the default.
opts.SetSupportedISA(InstructionSet_Base);
define HARDWARE_INTRINSIC_CLASS(flag, isa)                                                         \
    if (jitFlags.IsSet(JitFlags::flag))                                                            \
        opts.setSupportedISA(InstructionSet_##isa);
#include "hwintrinsiclistArm64.h"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍 Changing the order of lines would make it clearer.

@CarolEidt Are you also objecting to use of HARDWARE_INTRINSIC_CLASS_WITH_FLAG to remove InstructionSet_Base from the include list.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OK, I think I get it now - you can't omit InstructionSet_Base from hwintrinsiclistarm64.h because it's included elsewhere where it will be required. So, you need a way to handle that without the flag. I can't think of a better way to do this off the top of my head, but I definitely think that setting InstructionSet_Base first would help in understanding, and then perhaps if you changed HARDWARE_INTRINSIC_CLASS_WITH_FLAG to HARDWARE_INTRINSIC_CLASS_WITH_FLAG_ONLY it would make it clear that it's a boolean control that ignores the one without a flag.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed the HARDWARE_INTRINSIC_CLASS_WITH_FLAG and always handle base manually.

It is simpler and easier to understand.

@sdmaclea sdmaclea force-pushed the PR-ARM64-Set-Instruction-Set-Flags branch from ff74cd3 to 9874683 Compare January 19, 2018 18:37
@sdmaclea sdmaclea force-pushed the PR-ARM64-Set-Instruction-Set-Flags branch from 9874683 to 54b5fd0 Compare January 19, 2018 19:42
Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for making it clearer!

@CarolEidt CarolEidt merged commit 7f8c67e into dotnet:master Jan 19, 2018
@fiigii fiigii mentioned this pull request Jan 20, 2018
@sdmaclea sdmaclea deleted the PR-ARM64-Set-Instruction-Set-Flags branch May 24, 2018 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants