Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Paths

Table of Contentst

[flang][driver] Add standard predefinitions
ClosedPublic
Actions

Authored by FarisRehman on Jan 12 2021, 9:37 AM.

Details

Reviewers

sscalpone
tskeith
awarzynski
sameeranjoshi
AMDChirag
kiranktp
SouraVX

Commits

rG197d9a55f105: [flang][driver] Add standard macro predefinitions for compiler version

Summary

Add the following standard predefinitions that f18 supports: __flang__, __flang_major__, __flang__minor__, __flang_patchlevel__

Summary of changes:

Populate Fortran::parser::Options#predefinitions with the default supported predefinitions

Depends on: D94422

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

FarisRehman created this revision.Jan 12 2021, 9:37 AM

Herald added a reviewer: sscalpone. · View Herald TranscriptJan 12 2021, 9:37 AM

Herald added a subscriber: pengfei. · View Herald Transcript

FarisRehman requested review of this revision.Jan 12 2021, 9:37 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 9:37 AM

Herald added a subscriber: llvm-commits. · View Herald Transcript

FarisRehman edited the summary of this revision. (Show Details)Jan 12 2021, 9:38 AM

FarisRehman edited the summary of this revision. (Show Details)Jan 12 2021, 9:42 AM

FarisRehman added reviewers: tskeith, awarzynski, sameeranjoshi, AMDChirag, kiranktp, SouraVX.

Harbormaster completed remote builds in B84858: Diff 316137.Jan 12 2021, 10:11 AM

sameeranjoshi added inline comments.Jan 14 2021, 7:18 AM

flang/lib/Frontend/CompilerInvocation.cpp
272	Why is this `1` here? Also why only `x86_64`, why don't we add other architectures here?
flang/test/Flang-Driver/compiler-defined-macros.f90
24 ↗	(On Diff #316137)	How do you test `__x86_64__` ? I think we can add architecture specific tests in lit based test cases, does that make sense to add them here?

Thank you for working on this @FarisRehman !

@FarisRehman Re __X86_64__, in my opinion it is too early for any target-specific predefinitions and I suggest that we remove it for now. Lets revisit in the future when code-generation is supported.

@sameeranjoshi How does it sound? I totally agree with you that once we start adding such predefinitions, we should provide a solution that will support multiple architectures.

flang/lib/Frontend/CompilerInvocation.cpp
263	[nit] IMO this is fine for now, but long term the list of standard predefinitions is likely to grow significantly. Could add a TODO here? e.g. // TODO: When expanding this list of standard predefinitions, consider creating a dedicated API for this. Also, at some point we will need to differentiate between different targets.

FarisRehman mentioned this in D94805: [flang][driver] Update PP tests to use the new driver.Jan 15 2021, 10:02 AM

Agreed. @awarzynski. Thank you for clarification.

Remove target-specific predefinition

Remove a target-specific predefinition (__x86_64__), addressing review comments by @awarzynski and @sameeranjoshi

Summary of changes:

Remove predefinition __x86_64__
Add __flang__ to predefinitions regression test

FarisRehman edited the summary of this revision. (Show Details)Jan 18 2021, 3:47 AM

Harbormaster completed remote builds in B85581: Diff 317321.Jan 18 2021, 4:20 AM

Thank you.
LGTM.

This revision is now accepted and ready to land.Jan 18 2021, 5:11 AM

LGTM @FarisRehman , thank you!

flang/test/Flang-Driver/compiler-defined-macros.f90
1 ↗	(On Diff #317321)	[nit] IMHO we shouldn't be making a reference to `f18` here. This test simply verifies that `flang-new` correctly defines macro pre-definitions for the compiler version. Perhaps the file could be renamed to reflect that?