[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer abandoned this revision.
SjoerdMeijer added a comment.

Fair enough, perhaps the audience is too small here on llvm.org for this and 
this is too niche. In A-profile we have the same problem, so could the exercise 
for an A-core here, but can't spend time on that now, so will abandon this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78565/new/

https://reviews.llvm.org/D78565



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG35cf2f42dda4: [Driver][docs] Document option -mtune as a 
no-op. (authored by SjoerdMeijer).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78511/new/

https://reviews.llvm.org/D78511

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2721,7 +2721,8 @@
 def module_file_info : Flag<["-"], "module-file-info">, 
Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accepted for compatibility with GCC. Currently has no effect.">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepted for compatibility with GCC. Currently has no effect.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2721,7 +2721,8 @@
 def module_file_info : Flag<["-"], "module-file-info">, Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accepted for compatibility with GCC. Currently has no effect.">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepted for compatibility with GCC. Currently has no effect.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Hi Peter,
Thanks for reviewing again! I thought these examples to be relevant here, 
because it shows usage and examples of this tool (i.e. open-source clang/llvm). 
Additional benefits is that source and documentation is in one place, and it 
allows others, non-Arm people, to edit and review this document too. But I will 
take this question to the list, to see what people think about this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78565/new/

https://reviews.llvm.org/D78565



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added reviewers: psmith, ostannard, kristof.beyls, chill.
Herald added subscribers: danielkiss, arphaman.

Following the discussion about -mtune on the cfe dev list, I thought it would 
be good to make a start with documenting common command line arguments to 
target different ARM CPU and architecture combinations. This list is not yet 
complete, is work in progress, but I have just taken 2 recent M-cores as an 
example.


https://reviews.llvm.org/D78565

Files:
  clang/docs/ClangARMCPUsCLI.rst
  clang/docs/index.rst


Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -20,6 +20,7 @@
Toolchain
LanguageExtensions
ClangCommandLineReference
+   ClangARMCPUsCLI
AttributeReference
DiagnosticsReference
CrossCompilation
Index: clang/docs/ClangARMCPUsCLI.rst
===
--- /dev/null
+++ clang/docs/ClangARMCPUsCLI.rst
@@ -0,0 +1,77 @@
+=
+Clang ARM CPU command line argument reference
+=
+
+This page lists common command line arguments to target different ARM CPU
+and architecture combinations. This list is not yet complete, and is work in
+progress.
+
+Cortex-M33
+==
+
+Architecture: Armv8-M
+
+Technical Reference Manual:
+`http://infocenter.arm.com/help/topic/com.arm.doc.100230_0002_00_en/cortex_m33_trm_100230_0002_00_en.pdf`
+
+Optional architecture extensions:
+- single precision floating-point (FP),
+- DSP,
+- ARMv8-M Security Extension (CMSE),
+- Custom Datapath Extension (CDE).
+
+Example architecture configurations and corresponding CLI options:
+
+All extensions enabled, except CMSE and CDE, with DSP implied by 
-mcpu=cortex-m33:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=fp-armv8-sp-d16 
-mfloat-abi=hard -mthumb
+
+All extensions enabled:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=fp-armv8-sp-d16 
-mfloat-abi=hard -mthumb -mcmse
+
+Without single precision float support:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=none -mthumb
+
+Without single precision float and DSP support:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33+nodsp -mfpu=none 
-mthumb
+
+Cortex-M55
+==
+
+Architecture: Armv8.1-M Main
+
+Technical Reference Manual: not yet available.
+
+Optional architecture extensions:
+- M-Profile Vector Extension (MVE), integer-only, or also floating-point.
+- Scalar half, single, and double precision floating-point,
+- ARMv8-M Security Extension (CMSE),
+- Custom Datapath Extension (CDE).
+
+Mandatory architecture extensions (thus implied in the examples below):
+- DSP
+
+Example architecture configurations and corresponding CLI options:
+
+Integer and float MVE (INT, F16, F32), and float support (F16, F32, F64):
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m55 -mfloat-abi=hard 
-mthumb
+
+Integer-only MVE, with float support (F16, F32, F64):
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m55+nomve.fp 
-mfloat-abi=hard -mthumb
+
+Integer-only MVE, no float support:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m55+nofp -mfloat-abi=hard 
-mthumb
+
+No MVE, only float support (F16, F32, F64):
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m55+nomve -mfloat-abi=hard 
-mthumb
+
+Basic Armv8.1-M Main support, no extensions:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m55+nofp+nomve


Index: clang/docs/index.rst
===
--- clang/docs/index.rst
+++ clang/docs/index.rst
@@ -20,6 +20,7 @@
Toolchain
LanguageExtensions
ClangCommandLineReference
+   ClangARMCPUsCLI
AttributeReference
DiagnosticsReference
CrossCompilation
Index: clang/docs/ClangARMCPUsCLI.rst
===
--- /dev/null
+++ clang/docs/ClangARMCPUsCLI.rst
@@ -0,0 +1,77 @@
+=
+Clang ARM CPU command line argument reference
+=
+
+This page lists common command line arguments to target different ARM CPU
+and architecture combinations. This list is not yet complete, and is work in
+progress.
+
+Cortex-M33
+==
+
+Architecture: Armv8-M
+
+Technical Reference Manual:
+`http://infocenter.arm.com/help/topic/com.arm.doc.100230_0002_00_en/cortex_m33_trm_100230_0002_00_en.pdf`
+
+Optional architecture extensions:
+- single precision floating-point (FP),
+- DSP,
+- ARMv8-M Security Extension (CMSE),
+- Custom Datapath Extension (CDE).
+
+Example architecture configurations and corresponding CLI options:
+
+All extensions enabled, except CMSE and CDE, with DSP implied by -mcpu=cortex-m33:
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=fp-armv8-sp-d16 -mfloat-abi=hard 

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thank you both for reviewing!
And I will wait a day.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78511/new/

https://reviews.llvm.org/D78511



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 258823.
SjoerdMeijer added a comment.

Cheers, that's probably what I wanted to say.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78511/new/

https://reviews.llvm.org/D78511

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2710,7 +2710,8 @@
 def module_file_info : Flag<["-"], "module-file-info">, 
Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accepted for compatibility with GCC. Currently has no effect.">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepted for compatibility with GCC. Currently has no effect.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2710,7 +2710,8 @@
 def module_file_info : Flag<["-"], "module-file-info">, Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accepted for compatibility with GCC. Currently has no effect.">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepted for compatibility with GCC. Currently has no effect.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added a reviewer: dblaikie.

This is a doc change documenting that option -mtune does not perform any CPU 
type specific tuning but exists for compatability reasons with GCC.


https://reviews.llvm.org/D78511

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2710,7 +2710,9 @@
 def module_file_info : Flag<["-"], "module-file-info">, 
Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accept any  for compatability reasons with GCC, thus not "
+   "performing any CPU type specific tuning">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepts any value for compatability reasons with GCC, thus not performing any 
CPU type specific tuning.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2710,7 +2710,9 @@
 def module_file_info : Flag<["-"], "module-file-info">, Flags<[DriverOption,CC1Option]>, Group,
   HelpText<"Provide information about a particular module file">;
 def mthumb : Flag<["-"], "mthumb">, Group;
-def mtune_EQ : Joined<["-"], "mtune=">, Group;
+def mtune_EQ : Joined<["-"], "mtune=">, Group,
+  HelpText<"Accept any  for compatability reasons with GCC, thus not "
+   "performing any CPU type specific tuning">;
 def multi__module : Flag<["-"], "multi_module">;
 def multiply__defined__unused : Separate<["-"], "multiply_defined_unused">;
 def multiply__defined : Separate<["-"], "multiply_defined">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2735,6 +2735,8 @@
 .. option:: -mtune=
 .. program:: clang
 
+Accepts any value for compatability reasons with GCC, thus not performing any CPU type specific tuning.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77594: [SveEmitter] Add support for _n form builtins

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks reasonable to me




Comment at: clang/utils/TableGen/SveEmitter.cpp:212
+  bool hasSplat() const {
+return Proto.find_first_of("ajfrKLR") != std::string::npos;
+  }

"ajfrKLR" -> bingo ;-)

This probably makes sense, but who knows :-)
Not even sure if a comment makes things better here...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77594/new/

https://reviews.llvm.org/D77594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78401: [SveEmitter] IsInsertOp1SVALL and builtins for svqdec[bhwd] and svqinc[bhwd]

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks reasonable




Comment at: clang/include/clang/Basic/arm_sve.td:530
+class sat_type { string U = u; string T = t; }
+def SIGNED_BYTE : sat_type<"",  "c">;
+def SIGNED_HALF : sat_type<"",  "s">;

nit: just wondering if all these defs should be all capitals.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7684
 
+if (TypeFlags.isInsertOp1SVALL())
+  Ops.insert([1], Builder.getInt32(31));

would this be the most appropriate place to add the useful sentence from the 
description: 

"Some ACLE builtins leave out the argument to specify the predicate
pattern, which is expected to be expanded to an SV_ALL pattern."

because that's what 31 is, right?



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_qdecb.c:7
+#ifdef SVE_OVERLOADED_FORMS
+// A simple used,unused... macro, long enough to represent any SVE builtin.
+#define SVE_ACLE_FUNC(A1,A2_UNUSED,A3,A4_UNUSED) A1##A3

nit: used,unused -> "used/unused", or "used, unused"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78401/new/

https://reviews.llvm.org/D78401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a reviewer: efriedma.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

This is a big patch, but looks reasonable to me.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7451
+  }
+  llvm_unreachable("Unknown MemEltType");
+}

nit: to be consistent, do this in the default clause?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7458
+llvm_unreachable("Invalid SVETypeFlag!");
+
+  case SVETypeFlags::EltTyInt8:

nit: no need for the newlines here and also below?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7555
+
+  // From ACLE we always get . This might be incompatible with
+  // the actual type being loaded. Cast accordingly.

nit: can you rephrase this comment a bit I.e. the "From ACLE we always get ..." 
is a bit confusing I think. You want to say that this is how the ACLE defines 
this, but the IR looks different. You can also specify which bit is different, 
because that was not immediately obvious to me. 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77735/new/

https://reviews.llvm.org/D77735



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/docs/MatrixTypes.rst:27
+internal layout, overall size and alignment are implementation-defined.
+A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding
+enumeration types or an implementation-defined half-precision floating point

fhahn wrote:
> SjoerdMeijer wrote:
> > fhahn wrote:
> > > SjoerdMeijer wrote:
> > > > above you're using *element type* and here *matrix element type*. Since 
> > > > hopefully we're talking about the same things, "matrix *element type*" 
> > > > would be more consistent.
> > > > 
> > > > But this is just a nit, my main question is about the types:
> > > > why not e.g. define this to be the C11 types, that include _FloatN 
> > > > types, so that we can include N=16? Or is this intentionally omitted? I 
> > > > haven't even checked if this is supported in the architecture 
> > > > extension, but might make sense? And also, an element type cannot be an 
> > > > integer type? 
> > > > 
> > > >  
> > > > above you're using *element type* and here *matrix element type*. Since 
> > > > hopefully we're talking about the same things, "matrix *element type*" 
> > > > would be more consistent.
> > > 
> > > Yes it is referring to the same thing. I had a look at most uses, and in 
> > > most cases `element type` is used to refer to the element type of a given 
> > > matrix type. In that context it seems a bit verbose to use `matrix 
> > > element type`, although I am more than happy to change that if it helps 
> > > with clarifying things.
> > > 
> > > I intentionally used `matrix element type` in `Arithmetic Conversions`, 
> > > because there it is standing on its own and refers exactly to the set of 
> > > types defined as valid matrix element types here.
> > > 
> > > > why not e.g. define this to be the C11 types, that include _FloatN 
> > > > types, so that we can include N=16? Or is this intentionally omitted? I 
> > > > haven't even checked if this is supported in the architecture 
> > > > extension, but might make sense?
> > > 
> > > I couldn't find any reference to _FloatN types in the C11 draft version I 
> > > checked. Do you by any chance have a reference to the _FloatN types?
> > > 
> > > > And also, an element type cannot be an integer type?
> > > 
> > > The current definition should include it (real types include integer and 
> > > real floating point types according to  C99 6.2.5p17). I don't think 
> > > there is any reason to exclude them I think.
> > >>why not e.g. define this to be the C11 types, that include _FloatN 
> > >> types, so that we can include N=16? Or is this intentionally omitted? I 
> > >> haven't even checked if this is supported in the architecture extension, 
> > >> but might make sense?
> > >>
> > > I couldn't find any reference to _FloatN types in the C11 draft version I 
> > > checked. Do you by any chance have a reference to the _FloatN types?
> > 
> > Sorry, I was a bit imprecise here, it's an extension of C11: ISO/IEC TS 
> > 18661-3:2015.
> > My thinking was it would be cool to support the "proper" half-precision 
> > type. I thought about this, because of "or an implementation-defined 
> > half-precision" mentioned just below here, of which probably __fp16 is an 
> > example.  If you refer to the C99 types, you probably don't even need to 
> > mention this (although it won't do any harm)?
> > 
> > >>  And also, an element type cannot be an integer type?
> > >
> > > The current definition should include it (real types include integer and 
> > > real floating point types according to C99 6.2.5p17). I don't think there 
> > > is any reason to exclude them I think.
> > 
> > Ok, cheers, wrote this from memory (forgot this), and didn't check the 
> > standard.
> > Sorry, I was a bit imprecise here, it's an extension of C11: ISO/IEC TS 
> > 18661-3:2015.
> > My thinking was it would be cool to support the "proper" half-precision 
> > type. I thought about this, because of "or an implementation-defined 
> > half-precision" mentioned just below here, of which probably __fp16 is an 
> > example. If you refer to the C99 types, you probably don't even need to 
> > mention this (although it won't do any harm)?
> 
> I am not sure what the exact wording should be, but the intention is to 
> include both __fp16 and _Float16. I was hoping that would be covered as is, 
> but I would be happy to clarify (unfortunately it is not entirely clear to me 
> how to best word this)
Ah, okay, I got it. How about a simple enumeration, e.g.:  

  A *matrix element type* must be a C99 real type, excluding enumeration types, 
the C11 ISO/IEC TS 18661 _Float16 type, the ARM ACLE __fp16 type, or an 
implementation-defined half-precision floating point type, otherwise the 
program is ill-formed.



Comment at: clang/docs/MatrixTypes.rst:29
+A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding
+enumeration types or an 

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/docs/MatrixTypes.rst:27
+internal layout, overall size and alignment are implementation-defined.
+A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding
+enumeration types or an implementation-defined half-precision floating point

fhahn wrote:
> SjoerdMeijer wrote:
> > above you're using *element type* and here *matrix element type*. Since 
> > hopefully we're talking about the same things, "matrix *element type*" 
> > would be more consistent.
> > 
> > But this is just a nit, my main question is about the types:
> > why not e.g. define this to be the C11 types, that include _FloatN types, 
> > so that we can include N=16? Or is this intentionally omitted? I haven't 
> > even checked if this is supported in the architecture extension, but might 
> > make sense? And also, an element type cannot be an integer type? 
> > 
> >  
> > above you're using *element type* and here *matrix element type*. Since 
> > hopefully we're talking about the same things, "matrix *element type*" 
> > would be more consistent.
> 
> Yes it is referring to the same thing. I had a look at most uses, and in most 
> cases `element type` is used to refer to the element type of a given matrix 
> type. In that context it seems a bit verbose to use `matrix element type`, 
> although I am more than happy to change that if it helps with clarifying 
> things.
> 
> I intentionally used `matrix element type` in `Arithmetic Conversions`, 
> because there it is standing on its own and refers exactly to the set of 
> types defined as valid matrix element types here.
> 
> > why not e.g. define this to be the C11 types, that include _FloatN types, 
> > so that we can include N=16? Or is this intentionally omitted? I haven't 
> > even checked if this is supported in the architecture extension, but might 
> > make sense?
> 
> I couldn't find any reference to _FloatN types in the C11 draft version I 
> checked. Do you by any chance have a reference to the _FloatN types?
> 
> > And also, an element type cannot be an integer type?
> 
> The current definition should include it (real types include integer and real 
> floating point types according to  C99 6.2.5p17). I don't think there is any 
> reason to exclude them I think.
>>why not e.g. define this to be the C11 types, that include _FloatN types, 
>> so that we can include N=16? Or is this intentionally omitted? I haven't 
>> even checked if this is supported in the architecture extension, but might 
>> make sense?
>>
> I couldn't find any reference to _FloatN types in the C11 draft version I 
> checked. Do you by any chance have a reference to the _FloatN types?

Sorry, I was a bit imprecise here, it's an extension of C11: ISO/IEC TS 
18661-3:2015.
My thinking was it would be cool to support the "proper" half-precision type. I 
thought about this, because of "or an implementation-defined half-precision" 
mentioned just below here, of which probably __fp16 is an example.  If you 
refer to the C99 types, you probably don't even need to mention this (although 
it won't do any harm)?

>>  And also, an element type cannot be an integer type?
>
> The current definition should include it (real types include integer and real 
> floating point types according to C99 6.2.5p17). I don't think there is any 
> reason to exclude them I think.

Ok, cheers, wrote this from memory (forgot this), and didn't check the standard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76612/new/

https://reviews.llvm.org/D76612



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/docs/MatrixTypes.rst:12
+fixed-size matrices as language values and perform arithmetic on them.
+
+This feature is currently experimental, and both its design and its

Would it be good to set expectations here or in the section below: define that 
we're talking about 2-dimensional m × n matrices?



Comment at: clang/docs/MatrixTypes.rst:25
+element type, rows, and columns are the same type. A value of a matrix type
+includes storage for ``rows * columns`` values of the *element ype*. The
+internal layout, overall size and alignment are implementation-defined.

typo: ype -> type



Comment at: clang/docs/MatrixTypes.rst:27
+internal layout, overall size and alignment are implementation-defined.
+A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding
+enumeration types or an implementation-defined half-precision floating point

above you're using *element type* and here *matrix element type*. Since 
hopefully we're talking about the same things, "matrix *element type*" would be 
more consistent.

But this is just a nit, my main question is about the types:
why not e.g. define this to be the C11 types, that include _FloatN types, so 
that we can include N=16? Or is this intentionally omitted? I haven't even 
checked if this is supported in the architecture extension, but might make 
sense? And also, an element type cannot be an integer type? 

 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76612/new/

https://reviews.llvm.org/D76612



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/docs/MatrixSupport.rst:254
+
+Example
+===

Hi Florian, just reading this for the first time, this is cool stuff, and just 
a drive-by comment:

this section, Example, looks like a good candidate to be moved to the 
"Matrixes" section in 
clang/docs/LanguageExtensions.rst. This is Clang/LLVM specific, may not be that 
relevant for a language draft spec? But anyway, it may also be some of the 
user-facing examples missing in the language extension doc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76612/new/

https://reviews.llvm.org/D76612



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76678: [SveEmitter] Add range checks for immediates and predicate patterns.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

LGTM, please wait a day in case Eli has more comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76678/new/

https://reviews.llvm.org/D76678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76617: [SveEmitter] Fix encoding/decoding of SVETypeFlags

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Cheers, LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76617/new/

https://reviews.llvm.org/D76617



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76617: [SveEmitter] Fix encoding/decoding of SVETypeFlags

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/utils/TableGen/SveEmitter.cpp:229
+  // Returns the SVETypeFlags for a given value and mask.
+  unsigned encodeFlag(unsigned V, StringRef MaskName) const {
+auto It = FlagTypes.find(MaskName);

Should `V` now be an `uint64_t`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76617/new/

https://reviews.llvm.org/D76617



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76680: [SveEmitter] Add immediate checks for lanes and complex imms

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Looks good to me, but just one question about the tests. If I haven't 
overlooked anything, I don't see tests that check the new diagnostics:
"argument should be the value 90 or 270"
"argument should be the value 0,90,180 or 270"

Should they be here, or are they somewhere else?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9208
+def err_rotation_argument_to_cmla
+: Error<"argument should be the value 0,90,180 or 270">;
 def warn_neon_vector_initializer_non_portable : Warning<

A proper nit, perhaps some spaces here: "0,90,180".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76680/new/

https://reviews.llvm.org/D76680



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

I think the float16 discussion is an interesting one, but doesn't necessarily 
need to be done here. I am asking some questions offline, but if we ever come 
to a different opinion on it, then we can follow up so it's somewhat orthogonal 
to this change, and so this looks fine to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Just to clarify my last sentence:

> Now I am wondering why the ARM SVE ACLE is using float16_t, and not just 
> _Float16. Do you have any insights in that too perhaps?

What I meant to say is why SVE intrinsics are not using _Float16?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve 
-fallow-half-arguments-and-returns -fsyntax-only -verify -D__ARM_FEATURE_SVE %s
+

sdesmalen wrote:
> SjoerdMeijer wrote:
> > sdesmalen wrote:
> > > SjoerdMeijer wrote:
> > > > sdesmalen wrote:
> > > > > SjoerdMeijer wrote:
> > > > > > Just curious about the `-fallow-half-arguments-and-returns`, do you 
> > > > > > need that here?
> > > > > > 
> > > > > > And if not here, why do you need it elsewhere (looks enabled on all 
> > > > > > tests)?
> > > > > It's not needed for this test, but we've generated most of our tests 
> > > > > from the ACLE spec and the tests that use a scalar float16_t (== 
> > > > > __fp16) will need this, such as the ACLE intrinsic:
> > > > > 
> > > > >   svfloat16_t svadd_m(svbool_t, svfloat16_t, float16_t);
> > > > > 
> > > > > If you feel strongly about it, I could remove it from the other RUN 
> > > > > lines.
> > > > Well, I think this is my surprise then. Thinking out loud: we're 
> > > > talking SVE here, which always implies FP16. That's why I am surprised 
> > > > that we bother with a storage-type only type. Looking at the SVE ACLE I 
> > > > indeed see:
> > > > 
> > > >   float16_t equivalent to __fp16
> > > > 
> > > > where I was probably expecting:
> > > > 
> > > >   float16_t equivalent to _Float16
> > > > 
> > > > and with that everything would be sorted I guess, then we also don't 
> > > > need the hack^W workaround that is 
> > > > `-fallow-half-arguments-and-returns`. But maybe there is a good reason 
> > > > to use/choose `__fp16` that I don't see here. Probably worth a quick 
> > > > question for the ARM SVE ACLE, would you mind quickly checking?
> > > > 
> > > > 
> > > As just checked with @rsandifo-arm, the reason is that the definition of 
> > > `float16_t` has to be compatible with `arm_neon.h`, which uses `__fp16` 
> > > for both Clang and GCC.
> > I was suspecting it was compatability reasons, but perhaps not with 
> > `arm_neon.h`. So what exactly does it mean to be compatible with 
> > arm_neon.h? I mean, put simply and naively, if you target SVE, you include 
> > arm_sve.h, and go from there. How does that interact with arm_neon.h and 
> > why can float16_t not be a proper half type? 
> If you target SVE, you can still use Neon instructions, so it's still 
> possible to include arm_neon.h as well. If those have differing definitions 
> of float16_t, that may give trouble when using builtins from both the Neon 
> and SVE header files.
ok, thank, got it. So we are supporting this case:

   #include 
   #include 
   void foo () {
  neon_intrinsic();
  sve_intrinsic();
   }

Well, I find this very unfortunate, because it could have been so beautiful, 
and now we're still have this storage-only type while even the ACLE discourages 
its use. The use of `-fallow-half-arguments-and-returns` is just a minor 
annoyance, but point is it shouldn't have been necessary.

Now I am wondering why the ARM SVE ACLE  is using float16_t, and not just 
_Float16. Do you  have any insights in that too perhaps? 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve 
-fallow-half-arguments-and-returns -fsyntax-only -verify -D__ARM_FEATURE_SVE %s
+

sdesmalen wrote:
> SjoerdMeijer wrote:
> > sdesmalen wrote:
> > > SjoerdMeijer wrote:
> > > > Just curious about the `-fallow-half-arguments-and-returns`, do you 
> > > > need that here?
> > > > 
> > > > And if not here, why do you need it elsewhere (looks enabled on all 
> > > > tests)?
> > > It's not needed for this test, but we've generated most of our tests from 
> > > the ACLE spec and the tests that use a scalar float16_t (== __fp16) will 
> > > need this, such as the ACLE intrinsic:
> > > 
> > >   svfloat16_t svadd_m(svbool_t, svfloat16_t, float16_t);
> > > 
> > > If you feel strongly about it, I could remove it from the other RUN lines.
> > Well, I think this is my surprise then. Thinking out loud: we're talking 
> > SVE here, which always implies FP16. That's why I am surprised that we 
> > bother with a storage-type only type. Looking at the SVE ACLE I indeed see:
> > 
> >   float16_t equivalent to __fp16
> > 
> > where I was probably expecting:
> > 
> >   float16_t equivalent to _Float16
> > 
> > and with that everything would be sorted I guess, then we also don't need 
> > the hack^W workaround that is `-fallow-half-arguments-and-returns`. But 
> > maybe there is a good reason to use/choose `__fp16` that I don't see here. 
> > Probably worth a quick question for the ARM SVE ACLE, would you mind 
> > quickly checking?
> > 
> > 
> As just checked with @rsandifo-arm, the reason is that the definition of 
> `float16_t` has to be compatible with `arm_neon.h`, which uses `__fp16` for 
> both Clang and GCC.
I was suspecting it was compatability reasons, but perhaps not with 
`arm_neon.h`. So what exactly does it mean to be compatible with arm_neon.h? I 
mean, put simply and naively, if you target SVE, you include arm_sve.h, and go 
from there. How does that interact with arm_neon.h and why can float16_t not be 
a proper half type? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.






Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve 
-fallow-half-arguments-and-returns -fsyntax-only -verify -D__ARM_FEATURE_SVE %s
+

sdesmalen wrote:
> SjoerdMeijer wrote:
> > Just curious about the `-fallow-half-arguments-and-returns`, do you need 
> > that here?
> > 
> > And if not here, why do you need it elsewhere (looks enabled on all tests)?
> It's not needed for this test, but we've generated most of our tests from the 
> ACLE spec and the tests that use a scalar float16_t (== __fp16) will need 
> this, such as the ACLE intrinsic:
> 
>   svfloat16_t svadd_m(svbool_t, svfloat16_t, float16_t);
> 
> If you feel strongly about it, I could remove it from the other RUN lines.
Well, I think this is my surprise then. Thinking out loud: we're talking SVE 
here, which always implies FP16. That's why I am surprised that we bother with 
a storage-type only type. Looking at the SVE ACLE I indeed see:

  float16_t equivalent to __fp16

where I was probably expecting:

  float16_t equivalent to _Float16

and with that everything would be sorted I guess, then we also don't need the 
hack^W workaround that is `-fallow-half-arguments-and-returns`. But maybe there 
is a good reason to use/choose `__fp16` that I don't see here. Probably worth a 
quick question for the ARM SVE ACLE, would you mind quickly checking?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for the ping, hadn't noticed the updates.

I may have also missed why you need the SVE_ACLE_FUNC macro. Can you quickly 
comment why you need that? Just asking because in my opinion it doesn't really 
make it more pleasant to read (so it's there for another reason).

On one of the other tickets (may have missed a notification there too)  I asked 
why you need option `-fallow-half-arguments-and-returns`. Do you really need 
that, and why?

And last question is if you really need to pass these defines 
`-D__ARM_FEATURE_SVE`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76238/new/

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76062/new/

https://reviews.llvm.org/D76062



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76678: [SveEmitter] Add range checks for immediates and predicate patterns.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Did a first scan, looks very reasonable, just some first nits/questions inlined.




Comment at: clang/include/clang/Basic/arm_sve.td:153
+}
+def ImmCheckPredicatePattern: ImmCheckType<0>;  // 0..31
+def ImmCheck1_16: ImmCheckType<1>;  // 1..16

would it make sense to call this `ImmCheck0_31`?



Comment at: 
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_qdech.c:10
+{
+  return svqdech_pat_s16(op, SV_VL8, 0); // expected-error-re {{argument value 
{{[0-9]+}} is outside the valid range [1, 16]}}
+}

nit: why use a regexp for the argument value and not just its value?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76678/new/

https://reviews.llvm.org/D76678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve 
-fallow-half-arguments-and-returns -fsyntax-only -verify -D__ARM_FEATURE_SVE %s
+

Just curious about the `-fallow-half-arguments-and-returns`, do you need that 
here?

And if not here, why do you need it elsewhere (looks enabled on all tests)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76617: [SveEmitter] Fix encoding/decoding of SVETypeFlags

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Yes you're right, the patch that I've made dependent needs this one to work 
> properly.

Ah ok, I may have missed that, will have a look


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76617/new/

https://reviews.llvm.org/D76617



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76617: [SveEmitter] Fix encoding/decoding of SVETypeFlags

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Patches with functional changes but without tests are a bit "suspicious".  In 
this case, I see it might be tricky. You could argue that it should be 
incorporated in the patch that includes the tests, so we can see what's 
happening. But perhaps separating this out is cleaner, if the other patch is 
big. But can you at least make the next patch dependent on this one, so we know 
where this is used?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76617/new/

https://reviews.llvm.org/D76617



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks good to me. Please wait a day in case Eli has more comments.




Comment at: clang/include/clang/Basic/arm_sve.td:128
 def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
+def SVLD1SB : MInst<"svld1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
 MemEltTyInt8>;
+def SVLD1UB : MInst<"svld1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZxtReturn], MemEltTyInt8>;

sdesmalen wrote:
> SjoerdMeijer wrote:
> > nit: don't think we have a coding style for tablegen, but it is exceeding 
> > 80 characters, even making this on phabricator a bit difficult to read, 
> > perhaps you can reshuffle this a bit.
> I gave it a try to avoid these long lines, but found that this made the .td 
> file a lot less readable. If you don't have a strong opinion on this, I'd 
> prefer to keep it as-is.
Ok, thanks for trying.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1sh.c:5
+//
+// ld1sh
+//

Nit: I really don't mind this, but this comment in its current form it doesn't 
add much (might as well remove it, or add a description, same for the other 
files)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76238/new/

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Besides the irrelevant formatting nits, one minor question about the clang test.




Comment at: clang/test/Driver/aarch64-cpus.c:622
+
+// The BFloat16 extension is a mandatory component of the Armv8.6-A 
extensions, but is permitted as an
+// optional feature for any implementation of Armv8.2-A to Armv8.5-A 
(inclusive)

Do we need 2 additional tests here?
- one for v8.6,
- and another with SVE?





Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7806
+  def v4f16 : BaseSIMDThreeSameVectorBFDot<0, U, asm, ".2s", ".4h", V64,
+ v2f32, v8i8>;
+  def v8f16 : BaseSIMDThreeSameVectorBFDot<1, U, asm, ".4s", ".8h", V128,

nit: indentation is a bit off here



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7808
+  def v8f16 : BaseSIMDThreeSameVectorBFDot<1, U, asm, ".4s", ".8h", V128,
+ v4f32, v16i8>;
+}

here too



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7812
+class BaseSIMDThreeSameVectorBF16DotI {
+

and this can be on the same line as above?



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7867
+V128, asm, ".4s",
+  []> {
+  let AsmString = !strconcat(asm, "{\t$Rd", ".4s", ", $Rn", ".8h",

and perhaps this one. But looks intentional, perhaps it's fine then, I don't 
know.



Comment at: llvm/lib/Target/ARM/ARMInstrNEON.td:8936
+   N3RegFrm, IIC_VDOTPROD, "", "", []>
+{
+

on the same line as above?



Comment at: llvm/lib/Target/ARM/ARMInstrNEON.td:8937
+{
+
+let hasNoSchedulingInfo = 1;

no newline?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76062/new/

https://reviews.llvm.org/D76062



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7467
+
+  return IsZxtReturn ? Builder.CreateZExt(Load, VectorTy)
+ : Builder.CreateSExt(Load, VectorTy);

sdesmalen wrote:
> SjoerdMeijer wrote:
> > nit: and now looking at this, this can be a zero or sign extend, so `Zxt` 
> > is slightly misleading?
> The default is sign-extend, so we added a flag for the case where a 
> zero-extend is needed/expected. Are you suggesting to rename the flag or to 
> add an extra flag for IsSxt?
I don't know to be honest, it was a nit anyway, but whatever you think is best. 
But just reading the name `IsZxtReturn`, I was only expecting a zero-extend.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76238/new/

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Some nits inlined




Comment at: clang/include/clang/Basic/AArch64SVETypeFlags.h:72
+  bool isStructStore() const { return Flags & IsStructStore; }
+  bool isZxtReturn() const { return Flags & IsZxtReturn; }
+

nit: this one is non obvious (at least to me), so perhaps worth a comment what 
this is. I can guess that Zext means zero extending, but you know, the 
context...



Comment at: clang/include/clang/Basic/arm_sve.td:128
 def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
+def SVLD1SB : MInst<"svld1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
 MemEltTyInt8>;
+def SVLD1UB : MInst<"svld1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZxtReturn], MemEltTyInt8>;

nit: don't think we have a coding style for tablegen, but it is exceeding 80 
characters, even making this on phabricator a bit difficult to read, perhaps 
you can reshuffle this a bit.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7467
+
+  return IsZxtReturn ? Builder.CreateZExt(Load, VectorTy)
+ : Builder.CreateSExt(Load, VectorTy);

nit: and now looking at this, this can be a zero or sign extend, so `Zxt` is 
slightly misleading?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76238/new/

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

That sounds good and that seems to address the feedback given here, which I 
agree with. Just wanted to briefly add that while it already looks like most 
interested people are on this ticket, perhaps it good to also share the plan 
and design on the cfe dev list for visibility.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76077/new/

https://reviews.llvm.org/D76077



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:184
+  // Also include the Armv8.5 defines
+  // FIXME: Armv8.6 makes some extensions mandatory. Handle them here.
+  getTargetDefinesARMV85A(Opts, Builder);

Can you be more specific, what are we missing here?

Hmm, now I see the same above: "FIXME: Armv8.5 makes some extensions mandatory. 
Handle them here."
While you're at it, can you also change that?



Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:52
+  AArch64::AEK_RDM  | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
+  AArch64::AEK_SM4  | AArch64::AEK_SHA3 | AArch64::AEK_BF16|
+  AArch64::AEK_SHA2 | AArch64::AEK_AES  | AArch64::AEK_I8MM))

just double checking (because I can't remember): BF16 is a mandatory extension?



Comment at: llvm/lib/Target/ARM/ARM.td:532
+   "Support ARM v8.6a instructions",
+   [HasV8_5aOps, FeatureBF16]>;
+

it's implied here, so looks mandatory.



Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:260
 
+  /// HasBF16 - True if subtarget supports BFloat16 floating point
+  bool HasBF16 = false;

nit:  BFloat16 floating point ->  BFloat16 floating point operations


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76062/new/

https://reviews.llvm.org/D76062



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[Diffusion] rG825235c140e7: Revert "[Sema] Use the canonical type in function isVector"

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> I'm still not sure why __fp16, which is a storage-only type, is used for the 
> element type of float16x4_t if we want to avoid promotion to a float vector 
> type.

About the types: `__fp16` this is the older, storage-only type. So, just 
historically, I think this element type was used to describe the neon vectors, 
which translates to vectors with elements of i16 or f16, which all seems fine.

> Was there a discussion on llvm-dev or phabricator?

I don't think this is necessary, because this is how it's been from let's say 
the beginning.

> Can we change the element type of the vector to _Float16?

I don't see a benefit of doing this, and making this perhaps intrusive change.

> It seems that the original plan discussed on llvm-dev 
> (http://lists.llvm.org/pipermail/cfe-dev/2017-May/053768.html) was to 
> introduce _Float16 and use it instead of __fp16 for ARMv8.2-A, but the 
> subsequent patches that were committed are making NeonEmitter.cpp emit 
> typedefs of vectors that have __fp16 as the element type.

Then `_Float16` came along, which is a proper half type (not a storage only 
type), is defined in a C11 extension, and should be used for the ARMv8.2-A 
native half instructions; the semantics of `__fp16` and `_Float16` are 
different, and as a result using `__fp16` for the v8.2 half FP instructions is 
not an option, so it is not a matter of using `_Float16` instead of `__fp16`.

As I said, your problem wasn't clear to me, so let's look at your example:

> For example, in vneg_f16, __p0 is promoted to a vector of float before it's 
> negated.
> 
> __ai float16x4_t vneg_f16(float16x4_t __p0) {
> 
>   float16x4_t __ret;
>__ret = -__p0;
>return __ret;
> 
> }

when I compile this and emit llvm ir I get this:

  define dso_local <2 x i32> @vneg_f16(<2 x i32> %__p0.coerce) 
local_unnamed_addr #0 {
  entry:
%0 = bitcast <2 x i32> %__p0.coerce to <4 x half>
%fneg = fneg fast <4 x half> %0
%1 = bitcast <4 x half> %fneg to <2 x i32>
ret <2 x i32> %1
  }

so far so good I think: this shows a fneg on vector of halfs.

Where and when do you see problems? Can you describe this, and also provide 
your compile commands?


BRANCHES
  master, release/10.x

Users:
  ahatanak (Author)

https://reviews.llvm.org/rG825235c140e7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75861: [SVE] Generate overloaded functions for ACLE intrinsics.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

With the nit addressed, this LGTM




Comment at: clang/utils/TableGen/SveEmitter.cpp:634
 " *\n"
 " * Permission is hereby granted, free of charge, to any person "
 "obtaining "

SjoerdMeijer wrote:
> Unrelated to this change, I just spotted it because it looked a bit messy, 
> but is this the old copyright header? While we are at it, replace this with 
> the new one? Looks like e.g. in arm_mve.h, we do use the new one. 
Let's ignore this for now, we can fix this later if needed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75861/new/

https://reviews.llvm.org/D75861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75861: [SVE] Generate overloaded functions for ACLE intrinsics.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:362
+def TargetAArch64 : TargetArch<["aarch64"]>;
+def TargetARM_AArch64 : TargetArch;
 def TargetAVR : TargetArch<["avr"]>;

nit: don't thin we use underscores in names, and looking at examples below, I 
guess this should be `TargetAnyARM`



Comment at: clang/utils/TableGen/SveEmitter.cpp:105
 
+  std::string str() const;
+

nit: add a comment here too (for consistency)



Comment at: clang/utils/TableGen/SveEmitter.cpp:634
 " *\n"
 " * Permission is hereby granted, free of charge, to any person "
 "obtaining "

Unrelated to this change, I just spotted it because it looked a bit messy, but 
is this the old copyright header? While we are at it, replace this with the new 
one? Looks like e.g. in arm_mve.h, we do use the new one. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75861/new/

https://reviews.llvm.org/D75861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Cheers, I think this looks very reasonable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75470/new/

https://reviews.llvm.org/D75470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5292
+  { #NameBase, SVE::BI__builtin_sve_##NameBase, 0, 0, TypeModifier }
+static const NeonIntrinsicInfo AArch64SVEIntrinsicMap[] = {
+#define GET_SVE_LLVM_INTRINSIC_MAP

I am wondering if it is confusing/correct to use NeonInstrinsicInfo here?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7456
+
+  auto *Builtin = findNeonIntrinsicInMap(AArch64SVEIntrinsicMap, BuiltinID,
+ AArch64SVEIntrinsicsProvenSorted);

and the same here: findNeon...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75470/new/

https://reviews.llvm.org/D75470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a subscriber: simon_tatham.
SjoerdMeijer added a comment.

Adding @simon_tatham in case he feels wants to have a look too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75470/new/

https://reviews.llvm.org/D75470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/include/clang/Basic/arm_sve.td:121
+// Load one vector (scalar base)
+def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;

sdesmalen wrote:
> SjoerdMeijer wrote:
> > This encoding, e.g, this is  "csilUcUsUiUlhfd", is such a monstrosity. It's 
> > a very efficient encoding, but of course completely unreadable.  I know 
> > there is prior art, and know that this is how it's been done, but just 
> > curious if you have given it thoughts how to do this in a normal way, a bit 
> > more c++y.  I don't want to de-rail this work, but if we are adding a new 
> > emitter, perhaps now is the time to give it a thought, so was just curious. 
> >  
> Haha, its a bit of a monstrosity indeed. The only thing I can think of here 
> would be having something like:
> ```class TypeSpecs val> {
>   list v = val;
> }
> def All_Int_Float_Ty : TypeSpecs<["c", "s", "i", "l", "Uc", "Us", "Ul", "h", 
> "f", "d">;
> 
> def SVLD1 : Minst<"svld1[_{2}]", "dPc", All_Int_Float_Ty, [IsLoad]>;```
> 
> But I suspect this gets a bit awkward because of the many permutations, I 
> count more than 40. Not sure if that would really improve the readability.
I would personally welcome any improvement here, even the smallest. But if you 
think it's tricky, then fair enough!

I've managed to completely ignore the MVE intrinsics work so far, but 
understood there were some innovations here and there (e.g. in tablegen). 
Probably because it is dealing with similar problems: a lot of intrinsics, some 
of them overloaded with different types. I'm going to have a little look now to 
see if there's anything we can borrow from that, or if that is unrelated



Comment at: clang/utils/TableGen/SveEmitter.cpp:64
+Predicate(false), PredicatePattern(false), PrefetchOp(false),
+Bitwidth(128), ElementBitwidth(~0U), NumVectors(1) {
+if (!TS.empty())

sdesmalen wrote:
> SjoerdMeijer wrote:
> > why a default of 128? Will this gives problems for SVE implementions with> 
> > 128 bits?
> SVE vectors are n x 128bits, so the 128 is scalable here.
ah, okay, fair enough, didn't realise that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75470/new/

https://reviews.llvm.org/D75470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Big patch, I only did a first scan, but looks very reasonable in general. Just 
a first round of nits and 2 questions.




Comment at: clang/include/clang/Basic/arm_sve.td:121
+// Load one vector (scalar base)
+def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;

This encoding, e.g, this is  "csilUcUsUiUlhfd", is such a monstrosity. It's a 
very efficient encoding, but of course completely unreadable.  I know there is 
prior art, and know that this is how it's been done, but just curious if you 
have given it thoughts how to do this in a normal way, a bit more c++y.  I 
don't want to de-rail this work, but if we are adding a new emitter, perhaps 
now is the time to give it a thought, so was just curious.  



Comment at: clang/utils/TableGen/SveEmitter.cpp:1
 //===- SveEmitter.cpp - Generate arm_sve.h for use with clang -*- C++ -*-===//
 //

I wanted to add the nit that SveEmiiter.cpp should perhaps be SVEEmitter.cpp, 
but then I saw at the bottom that MVE is spelled Mve, so perhaps this is fine 
then.



Comment at: clang/utils/TableGen/SveEmitter.cpp:64
+Predicate(false), PredicatePattern(false), PrefetchOp(false),
+Bitwidth(128), ElementBitwidth(~0U), NumVectors(1) {
+if (!TS.empty())

why a default of 128? Will this gives problems for SVE implementions with> 128 
bits?



Comment at: clang/utils/TableGen/SveEmitter.cpp:119
+class Intrinsic {
+  /// The Record this intrinsic was created from.
+  Record *R;

nit: for readability, perhaps don't abbreviate some of these member names? 

 R -> Record
 BaseTS -> BaseTypeSpec
 CK -> ClassKind



Comment at: clang/utils/TableGen/SveEmitter.cpp:263
+unsigned SVEType::getTypeFlags() const {
+  unsigned Base = 0;
+

don't need this



Comment at: clang/utils/TableGen/SveEmitter.cpp:267
+switch (ElementBitwidth) {
+case 16: Base = (unsigned)SVETypeFlags::Float16; break;
+case 32: Base = (unsigned)SVETypeFlags::Float32; break;

just a return here 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75470/new/

https://reviews.llvm.org/D75470



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Patch for the test-suite: D75557 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Ha, these test-suite apps fail with `multiple definition of ...` link errors, 
so actually require a little bit of porting! :-)
But I think I will prepare a patch that adds `-fcommon` to their makefiles, as 
I don't want to change too many things at the same time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Reverted in: https://reviews.llvm.org/rG4e363563fa14

Put up for review some fixes for compiler-rt tests in: 
https://reviews.llvm.org/D75520

Now checking what these test-suite failures are about:

  FAIL: MultiSource/Applications/JM/ldecod/ldecod.compile_time (1 of 2630)
  FAIL: MultiSource/Applications/JM/lencod/lencod.compile_time (2 of 2630)
  FAIL: MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk.compile_time (3 of 2630)
  FAIL: MultiSource/Benchmarks/DOE-ProxyApps-C/miniAMR/miniAMR.compile_time (4 
of 2630)
  FAIL: MultiSource/Benchmarks/Olden/bh/bh.compile_time (5 of 2630)
  FAIL: 
MultiSource/Benchmarks/Prolangs-C/TimberWolfMC/timberwolfmc.compile_time (6 of 
2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/compiler/compiler.compile_time (7 of 
2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/loader/loader.compile_time (8 of 2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/simulator/simulator.compile_time (9 
of 2630)
  FAIL: MultiSource/Applications/JM/ldecod/ldecod.execution_time (527 of 2630)
  FAIL: MultiSource/Applications/JM/lencod/lencod.execution_time (528 of 2630)
  FAIL: MultiSource/Benchmarks/ASC_Sequoia/IRSmk/IRSmk.execution_time (529 of 
2630)
  FAIL: MultiSource/Benchmarks/DOE-ProxyApps-C/miniAMR/miniAMR.execution_time 
(530 of 2630)
  FAIL: MultiSource/Benchmarks/Olden/bh/bh.execution_time (531 of 2630)
  FAIL: 
MultiSource/Benchmarks/Prolangs-C/TimberWolfMC/timberwolfmc.execution_time (532 
of 2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/compiler/compiler.execution_time (533 
of 2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/loader/loader.execution_time (534 of 
2630)
  FAIL: MultiSource/Benchmarks/Prolangs-C/simulator/simulator.execution_time 
(535 of 2630)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0a9fc9233e17: [Driver] Default to -fno-common for all 
targets (authored by SjoerdMeijer).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D75056?vs=246917=247822#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/2008-07-21-mixed-var-fn-decl.c
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/CodeGen/aarch64-sve.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-systemz.c
  clang/test/CodeGen/alignment.c
  clang/test/CodeGen/asm-label.c
  clang/test/CodeGen/attr-weak-import.c
  clang/test/CodeGen/attr-weakref2.c
  clang/test/CodeGen/attributes.c
  clang/test/CodeGen/blocks-windows.c
  clang/test/CodeGen/bool-convert.c
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGen/cfstring-elf-cfbuild-x86_64.c
  clang/test/CodeGen/cfstring-windows.c
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGen/dllexport-1.c
  clang/test/CodeGen/dllexport.c
  clang/test/CodeGen/dllimport.c
  clang/test/CodeGen/microsoft-no-common-align.c
  clang/test/CodeGen/no-common.c
  clang/test/CodeGen/pr25786.c
  clang/test/CodeGen/pragma-pack-1.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/private-extern-redef.c
  clang/test/CodeGen/tentative-decls.c
  clang/test/CodeGen/tls-model.c
  clang/test/CodeGen/visibility.c
  clang/test/CodeGen/vlt_to_pointer.c
  clang/test/CodeGen/volatile-1.c
  clang/test/CodeGen/weak-global.c
  clang/test/CodeGen/windows-on-arm-dllimport-dllexport.c
  clang/test/CodeGenCXX/clang-sections-tentative.c
  clang/test/CodeGenObjC/constant-string-class.m
  clang/test/CodeGenObjC/tentative-cfconstantstring.m
  clang/test/CodeGenOpenCL/address-spaces.cl
  clang/test/CodeGenOpenCL/amdgcn-large-globals.cl
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
  clang/test/Driver/apple-kext-mkernel.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fuchsia.c
  clang/test/Driver/no-common.c
  clang/test/Driver/xcore-opts.c
  clang/test/Frontend/ast-codegen.c
  clang/test/Headers/xmmintrin.c
  clang/test/PCH/chain-external-defs.c
  clang/test/PCH/external-defs.c
  clang/test/PCH/tentative-defs.c
  clang/test/Parser/pragma-visibility2.c

Index: clang/test/Parser/pragma-visibility2.c
===
--- clang/test/Parser/pragma-visibility2.c
+++ clang/test/Parser/pragma-visibility2.c
@@ -6,14 +6,14 @@
 #pragma GCC visibility push(hidden)
 
 int v1;
-// CHECK: @v1 = common hidden global i32 0, align 4
+// CHECK: @v1 = hidden global i32 0, align 4
 
 #pragma GCC visibility pop
 
 int v2;
-// CHECK: @v2 = common global i32 0, align 4
+// CHECK: @v2 = global i32 0, align 4
 
 _Pragma("GCC visibility push(hidden)");
 
 int v3;
-// CHECK: @v3 = common hidden global i32 0, align 4
+// CHECK: @v3 = hidden global i32 0, align 4
Index: clang/test/PCH/tentative-defs.c
===
--- clang/test/PCH/tentative-defs.c
+++ clang/test/PCH/tentative-defs.c
@@ -1,6 +1,6 @@
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/tentative-defs.h
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -verify -emit-llvm -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -emit-pch -o %t.pch %S/tentative-defs.h
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -include-pch %t.pch -verify -emit-llvm -o %t %s
 // REQUIRES: x86-registered-target
 
 // RUN: grep "@variable = common global i32 0" %t | count 1
Index: clang/test/PCH/external-defs.c
===
--- clang/test/PCH/external-defs.c
+++ clang/test/PCH/external-defs.c
@@ -3,16 +3,16 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/external-defs.h
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -emit-llvm -o %t %s
 
-// RUN: grep "@x = common global i32 0" %t | count 1
+// RUN: grep "@x = global i32 0" %t | count 1
 // RUN: not grep "@z" %t
 
 // RUN: grep "@x2 = global i32 19" %t | count 1
 int x2 = 19;
 
-// RUN: grep "@incomplete_array = common global .*1 x i32" %t | count 1
-// RUN: grep "@incomplete_array2 = common global .*17 x i32" %t | count 1
+// RUN: grep "@incomplete_array = global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array2 = global .*17 x i32" %t | count 1
 int incomplete_array2[17];
-// RUN: grep "@incomplete_array3 = common global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array3 = global .*1 x i32" %t | count 1
 int incomplete_array3[];
 
 struct S {
Index: 

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks all for the reviews and help.
I will fix these things and do a last rebase/check before committing this 
tomorrow.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Friendly ping, and just checking: are we happy with this? Good to go?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246917.
SjoerdMeijer edited the summary of this revision.
SjoerdMeijer added a comment.

> Are there any tests remaining that check that with -fcommon, IR generation 
> creates "common" variables, now that all these tests have been modified?

I've added a RUN line to `clang/test/CodeGen/no-common.c` with `-fcommon` that 
should test this.

> Suggestion:

Therefore, C code that uses tentative definitions as definitions of a variable 
in multiple translation units will trigger multiple-definition linker errors. 
Generally, this occurs when the use of the extern keyword is neglected in the 
declaration of a variable in a header file. In some cases, no specific 
translation unit provides a definition of the variable. The previous behavior 
can be restored by specifying -fcommon.

Thanks for your suggestion. I've verbatim added this to the release note.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/2008-07-21-mixed-var-fn-decl.c
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/CodeGen/aarch64-sve.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-systemz.c
  clang/test/CodeGen/alignment.c
  clang/test/CodeGen/asm-label.c
  clang/test/CodeGen/attr-weak-import.c
  clang/test/CodeGen/attr-weakref2.c
  clang/test/CodeGen/attributes.c
  clang/test/CodeGen/blocks-windows.c
  clang/test/CodeGen/bool-convert.c
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGen/cfstring-elf-cfbuild-x86_64.c
  clang/test/CodeGen/cfstring-windows.c
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGen/dllexport-1.c
  clang/test/CodeGen/dllexport.c
  clang/test/CodeGen/dllimport.c
  clang/test/CodeGen/microsoft-no-common-align.c
  clang/test/CodeGen/no-common.c
  clang/test/CodeGen/pr25786.c
  clang/test/CodeGen/pragma-pack-1.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/private-extern-redef.c
  clang/test/CodeGen/tentative-decls.c
  clang/test/CodeGen/tls-model.c
  clang/test/CodeGen/visibility.c
  clang/test/CodeGen/vlt_to_pointer.c
  clang/test/CodeGen/volatile-1.c
  clang/test/CodeGen/weak-global.c
  clang/test/CodeGen/windows-on-arm-dllimport-dllexport.c
  clang/test/CodeGenCXX/clang-sections-tentative.c
  clang/test/CodeGenObjC/constant-string-class.m
  clang/test/CodeGenObjC/tentative-cfconstantstring.m
  clang/test/CodeGenOpenCL/address-spaces.cl
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
  clang/test/Driver/apple-kext-mkernel.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fuchsia.c
  clang/test/Driver/no-common.c
  clang/test/Driver/xcore-opts.c
  clang/test/Frontend/ast-codegen.c
  clang/test/Headers/xmmintrin.c
  clang/test/PCH/chain-external-defs.c
  clang/test/PCH/external-defs.c
  clang/test/PCH/tentative-defs.c
  clang/test/Parser/pragma-visibility2.c

Index: clang/test/Parser/pragma-visibility2.c
===
--- clang/test/Parser/pragma-visibility2.c
+++ clang/test/Parser/pragma-visibility2.c
@@ -6,14 +6,14 @@
 #pragma GCC visibility push(hidden)
 
 int v1;
-// CHECK: @v1 = common hidden global i32 0, align 4
+// CHECK: @v1 = hidden global i32 0, align 4
 
 #pragma GCC visibility pop
 
 int v2;
-// CHECK: @v2 = common global i32 0, align 4
+// CHECK: @v2 = global i32 0, align 4
 
 _Pragma("GCC visibility push(hidden)");
 
 int v3;
-// CHECK: @v3 = common hidden global i32 0, align 4
+// CHECK: @v3 = hidden global i32 0, align 4
Index: clang/test/PCH/tentative-defs.c
===
--- clang/test/PCH/tentative-defs.c
+++ clang/test/PCH/tentative-defs.c
@@ -1,6 +1,6 @@
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/tentative-defs.h
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -verify -emit-llvm -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -emit-pch -o %t.pch %S/tentative-defs.h
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -include-pch %t.pch -verify -emit-llvm -o %t %s
 // REQUIRES: x86-registered-target
 
 // RUN: grep "@variable = common global i32 0" %t | count 1
Index: clang/test/PCH/external-defs.c
===
--- clang/test/PCH/external-defs.c
+++ clang/test/PCH/external-defs.c
@@ -3,16 +3,16 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/external-defs.h
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -emit-llvm -o %t %s
 
-// RUN: grep "@x = common global i32 0" %t | count 1
+// RUN: grep "@x = global i32 0" %t | count 1
 // RUN: not grep "@z" %t
 
 // RUN: grep "@x2 = global i32 19" %t | count 1
 

[PATCH] D75056: [Driver] Default to -fno-common for all targets

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246808.
SjoerdMeijer retitled this revision from "[Driver] Default to -fno-common" to 
"[Driver] Default to -fno-common for all targets".
SjoerdMeijer edited the summary of this revision.
SjoerdMeijer added a comment.

Removed the CHECK-NOTs from some tests as suggested.

And as also suggested, made the title more descriptive.

Thanks for all suggestions!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/2008-07-21-mixed-var-fn-decl.c
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/CodeGen/aarch64-sve.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-systemz.c
  clang/test/CodeGen/alignment.c
  clang/test/CodeGen/asm-label.c
  clang/test/CodeGen/attr-weak-import.c
  clang/test/CodeGen/attr-weakref2.c
  clang/test/CodeGen/attributes.c
  clang/test/CodeGen/blocks-windows.c
  clang/test/CodeGen/bool-convert.c
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGen/cfstring-elf-cfbuild-x86_64.c
  clang/test/CodeGen/cfstring-windows.c
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGen/dllexport-1.c
  clang/test/CodeGen/dllexport.c
  clang/test/CodeGen/dllimport.c
  clang/test/CodeGen/microsoft-no-common-align.c
  clang/test/CodeGen/no-common.c
  clang/test/CodeGen/pr25786.c
  clang/test/CodeGen/pragma-pack-1.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/private-extern-redef.c
  clang/test/CodeGen/tentative-decls.c
  clang/test/CodeGen/tls-model.c
  clang/test/CodeGen/visibility.c
  clang/test/CodeGen/vlt_to_pointer.c
  clang/test/CodeGen/volatile-1.c
  clang/test/CodeGen/weak-global.c
  clang/test/CodeGen/windows-on-arm-dllimport-dllexport.c
  clang/test/CodeGenCXX/clang-sections-tentative.c
  clang/test/CodeGenObjC/constant-string-class.m
  clang/test/CodeGenObjC/tentative-cfconstantstring.m
  clang/test/CodeGenOpenCL/address-spaces.cl
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
  clang/test/Driver/apple-kext-mkernel.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fuchsia.c
  clang/test/Driver/no-common.c
  clang/test/Driver/xcore-opts.c
  clang/test/Frontend/ast-codegen.c
  clang/test/Headers/xmmintrin.c
  clang/test/PCH/chain-external-defs.c
  clang/test/PCH/external-defs.c
  clang/test/PCH/tentative-defs.c
  clang/test/Parser/pragma-visibility2.c

Index: clang/test/Parser/pragma-visibility2.c
===
--- clang/test/Parser/pragma-visibility2.c
+++ clang/test/Parser/pragma-visibility2.c
@@ -6,14 +6,14 @@
 #pragma GCC visibility push(hidden)
 
 int v1;
-// CHECK: @v1 = common hidden global i32 0, align 4
+// CHECK: @v1 = hidden global i32 0, align 4
 
 #pragma GCC visibility pop
 
 int v2;
-// CHECK: @v2 = common global i32 0, align 4
+// CHECK: @v2 = global i32 0, align 4
 
 _Pragma("GCC visibility push(hidden)");
 
 int v3;
-// CHECK: @v3 = common hidden global i32 0, align 4
+// CHECK: @v3 = hidden global i32 0, align 4
Index: clang/test/PCH/tentative-defs.c
===
--- clang/test/PCH/tentative-defs.c
+++ clang/test/PCH/tentative-defs.c
@@ -1,6 +1,6 @@
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/tentative-defs.h
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -verify -emit-llvm -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -emit-pch -o %t.pch %S/tentative-defs.h
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -include-pch %t.pch -verify -emit-llvm -o %t %s
 // REQUIRES: x86-registered-target
 
 // RUN: grep "@variable = common global i32 0" %t | count 1
Index: clang/test/PCH/external-defs.c
===
--- clang/test/PCH/external-defs.c
+++ clang/test/PCH/external-defs.c
@@ -3,16 +3,16 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/external-defs.h
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -emit-llvm -o %t %s
 
-// RUN: grep "@x = common global i32 0" %t | count 1
+// RUN: grep "@x = global i32 0" %t | count 1
 // RUN: not grep "@z" %t
 
 // RUN: grep "@x2 = global i32 19" %t | count 1
 int x2 = 19;
 
-// RUN: grep "@incomplete_array = common global .*1 x i32" %t | count 1
-// RUN: grep "@incomplete_array2 = common global .*17 x i32" %t | count 1
+// RUN: grep "@incomplete_array = global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array2 = global .*17 x i32" %t | count 1
 int incomplete_array2[17];
-// RUN: grep "@incomplete_array3 = common global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array3 = global .*1 x i32" %t | count 1
 int incomplete_array3[];
 
 struct S 

[PATCH] D75056: [Driver] Default to -fno-common

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246768.
SjoerdMeijer added a comment.
Herald added subscribers: kerbowa, nhaehnle, jvesely.

Thanks for catching that. Now it shows more changes in tests, so updated a 
bunch of tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/2008-07-21-mixed-var-fn-decl.c
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/CodeGen/aarch64-sve.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-systemz.c
  clang/test/CodeGen/alignment.c
  clang/test/CodeGen/asm-label.c
  clang/test/CodeGen/attr-weak-import.c
  clang/test/CodeGen/attr-weakref2.c
  clang/test/CodeGen/attributes.c
  clang/test/CodeGen/blocks-windows.c
  clang/test/CodeGen/bool-convert.c
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGen/cfstring-elf-cfbuild-x86_64.c
  clang/test/CodeGen/cfstring-windows.c
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGen/dllexport-1.c
  clang/test/CodeGen/dllexport.c
  clang/test/CodeGen/dllimport.c
  clang/test/CodeGen/microsoft-no-common-align.c
  clang/test/CodeGen/no-common.c
  clang/test/CodeGen/pr25786.c
  clang/test/CodeGen/pragma-pack-1.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/private-extern-redef.c
  clang/test/CodeGen/tentative-decls.c
  clang/test/CodeGen/tls-model.c
  clang/test/CodeGen/visibility.c
  clang/test/CodeGen/vlt_to_pointer.c
  clang/test/CodeGen/volatile-1.c
  clang/test/CodeGen/weak-global.c
  clang/test/CodeGen/windows-on-arm-dllimport-dllexport.c
  clang/test/CodeGenCXX/clang-sections-tentative.c
  clang/test/CodeGenObjC/constant-string-class.m
  clang/test/CodeGenObjC/tentative-cfconstantstring.m
  clang/test/CodeGenOpenCL/address-spaces.cl
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
  clang/test/Driver/apple-kext-mkernel.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fuchsia.c
  clang/test/Driver/no-common.c
  clang/test/Driver/xcore-opts.c
  clang/test/Frontend/ast-codegen.c
  clang/test/Headers/xmmintrin.c
  clang/test/PCH/chain-external-defs.c
  clang/test/PCH/external-defs.c
  clang/test/PCH/tentative-defs.c
  clang/test/Parser/pragma-visibility2.c

Index: clang/test/Parser/pragma-visibility2.c
===
--- clang/test/Parser/pragma-visibility2.c
+++ clang/test/Parser/pragma-visibility2.c
@@ -6,14 +6,14 @@
 #pragma GCC visibility push(hidden)
 
 int v1;
-// CHECK: @v1 = common hidden global i32 0, align 4
+// CHECK: @v1 = hidden global i32 0, align 4
 
 #pragma GCC visibility pop
 
 int v2;
-// CHECK: @v2 = common global i32 0, align 4
+// CHECK: @v2 = global i32 0, align 4
 
 _Pragma("GCC visibility push(hidden)");
 
 int v3;
-// CHECK: @v3 = common hidden global i32 0, align 4
+// CHECK: @v3 = hidden global i32 0, align 4
Index: clang/test/PCH/tentative-defs.c
===
--- clang/test/PCH/tentative-defs.c
+++ clang/test/PCH/tentative-defs.c
@@ -1,6 +1,6 @@
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/tentative-defs.h
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -verify -emit-llvm -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -emit-pch -o %t.pch %S/tentative-defs.h
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -include-pch %t.pch -verify -emit-llvm -o %t %s
 // REQUIRES: x86-registered-target
 
 // RUN: grep "@variable = common global i32 0" %t | count 1
Index: clang/test/PCH/external-defs.c
===
--- clang/test/PCH/external-defs.c
+++ clang/test/PCH/external-defs.c
@@ -3,16 +3,16 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/external-defs.h
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -emit-llvm -o %t %s
 
-// RUN: grep "@x = common global i32 0" %t | count 1
+// RUN: grep "@x = global i32 0" %t | count 1
 // RUN: not grep "@z" %t
 
 // RUN: grep "@x2 = global i32 19" %t | count 1
 int x2 = 19;
 
-// RUN: grep "@incomplete_array = common global .*1 x i32" %t | count 1
-// RUN: grep "@incomplete_array2 = common global .*17 x i32" %t | count 1
+// RUN: grep "@incomplete_array = global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array2 = global .*17 x i32" %t | count 1
 int incomplete_array2[17];
-// RUN: grep "@incomplete_array3 = common global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array3 = global .*1 x i32" %t | count 1
 int incomplete_array3[];
 
 struct S {
Index: clang/test/PCH/chain-external-defs.c
===
--- clang/test/PCH/chain-external-defs.c
+++ 

[PATCH] D75056: [Driver] Default to -fno-common

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246770.
SjoerdMeijer added a comment.

minor test update


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/2008-07-21-mixed-var-fn-decl.c
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/CodeGen/aarch64-sve.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-systemz.c
  clang/test/CodeGen/alignment.c
  clang/test/CodeGen/asm-label.c
  clang/test/CodeGen/attr-weak-import.c
  clang/test/CodeGen/attr-weakref2.c
  clang/test/CodeGen/attributes.c
  clang/test/CodeGen/blocks-windows.c
  clang/test/CodeGen/bool-convert.c
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGen/cfstring-elf-cfbuild-x86_64.c
  clang/test/CodeGen/cfstring-windows.c
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGen/dllexport-1.c
  clang/test/CodeGen/dllexport.c
  clang/test/CodeGen/dllimport.c
  clang/test/CodeGen/microsoft-no-common-align.c
  clang/test/CodeGen/no-common.c
  clang/test/CodeGen/pr25786.c
  clang/test/CodeGen/pragma-pack-1.c
  clang/test/CodeGen/pragma-weak.c
  clang/test/CodeGen/private-extern-redef.c
  clang/test/CodeGen/tentative-decls.c
  clang/test/CodeGen/tls-model.c
  clang/test/CodeGen/visibility.c
  clang/test/CodeGen/vlt_to_pointer.c
  clang/test/CodeGen/volatile-1.c
  clang/test/CodeGen/weak-global.c
  clang/test/CodeGen/windows-on-arm-dllimport-dllexport.c
  clang/test/CodeGenCXX/clang-sections-tentative.c
  clang/test/CodeGenObjC/constant-string-class.m
  clang/test/CodeGenObjC/tentative-cfconstantstring.m
  clang/test/CodeGenOpenCL/address-spaces.cl
  clang/test/CodeGenOpenCL/amdgpu-nullptr.cl
  clang/test/Driver/apple-kext-mkernel.c
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fuchsia.c
  clang/test/Driver/no-common.c
  clang/test/Driver/xcore-opts.c
  clang/test/Frontend/ast-codegen.c
  clang/test/Headers/xmmintrin.c
  clang/test/PCH/chain-external-defs.c
  clang/test/PCH/external-defs.c
  clang/test/PCH/tentative-defs.c
  clang/test/Parser/pragma-visibility2.c

Index: clang/test/Parser/pragma-visibility2.c
===
--- clang/test/Parser/pragma-visibility2.c
+++ clang/test/Parser/pragma-visibility2.c
@@ -6,14 +6,14 @@
 #pragma GCC visibility push(hidden)
 
 int v1;
-// CHECK: @v1 = common hidden global i32 0, align 4
+// CHECK: @v1 = hidden global i32 0, align 4
 
 #pragma GCC visibility pop
 
 int v2;
-// CHECK: @v2 = common global i32 0, align 4
+// CHECK: @v2 = global i32 0, align 4
 
 _Pragma("GCC visibility push(hidden)");
 
 int v3;
-// CHECK: @v3 = common hidden global i32 0, align 4
+// CHECK: @v3 = hidden global i32 0, align 4
Index: clang/test/PCH/tentative-defs.c
===
--- clang/test/PCH/tentative-defs.c
+++ clang/test/PCH/tentative-defs.c
@@ -1,6 +1,6 @@
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/tentative-defs.h
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -verify -emit-llvm -o %t %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -emit-pch -o %t.pch %S/tentative-defs.h
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fcommon -include-pch %t.pch -verify -emit-llvm -o %t %s
 // REQUIRES: x86-registered-target
 
 // RUN: grep "@variable = common global i32 0" %t | count 1
Index: clang/test/PCH/external-defs.c
===
--- clang/test/PCH/external-defs.c
+++ clang/test/PCH/external-defs.c
@@ -3,16 +3,16 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -emit-pch -o %t.pch %S/external-defs.h
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -include-pch %t.pch -emit-llvm -o %t %s
 
-// RUN: grep "@x = common global i32 0" %t | count 1
+// RUN: grep "@x = global i32 0" %t | count 1
 // RUN: not grep "@z" %t
 
 // RUN: grep "@x2 = global i32 19" %t | count 1
 int x2 = 19;
 
-// RUN: grep "@incomplete_array = common global .*1 x i32" %t | count 1
-// RUN: grep "@incomplete_array2 = common global .*17 x i32" %t | count 1
+// RUN: grep "@incomplete_array = global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array2 = global .*17 x i32" %t | count 1
 int incomplete_array2[17];
-// RUN: grep "@incomplete_array3 = common global .*1 x i32" %t | count 1
+// RUN: grep "@incomplete_array3 = global .*1 x i32" %t | count 1
 int incomplete_array3[];
 
 struct S {
Index: clang/test/PCH/chain-external-defs.c
===
--- clang/test/PCH/chain-external-defs.c
+++ clang/test/PCH/chain-external-defs.c
@@ -16,11 +16,11 @@
 
 // Z-NOT: @z
 
-// XA: @x = common global i32 0
-// XA-NOT: @x = common global 

[PATCH] D75056: [Driver] Default to -fno-common

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246491.
SjoerdMeijer added a comment.

Thanks, have added a note to the release notes, and also to the command line 
reference.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/ReleaseNotes.rst
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/Driver/no-common.c
  clang/test/Driver/rewrite-legacy-objc.m
  clang/test/Driver/rewrite-objc.m
  clang/test/Frontend/ast-codegen.c

Index: clang/test/Frontend/ast-codegen.c
===
--- clang/test/Frontend/ast-codegen.c
+++ clang/test/Frontend/ast-codegen.c
@@ -5,7 +5,7 @@
 // CHECK: module asm "foo"
 __asm__("foo");
 
-// CHECK: @g0 = common dso_local global i32 0, align 4
+// CHECK: @g0 = dso_local global i32 0, align 4
 int g0;
 
 // CHECK: define dso_local i32 @f0()
Index: clang/test/Driver/rewrite-objc.m
===
--- clang/test/Driver/rewrite-objc.m
+++ clang/test/Driver/rewrite-objc.m
@@ -3,4 +3,4 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
Index: clang/test/Driver/rewrite-legacy-objc.m
===
--- clang/test/Driver/rewrite-legacy-objc.m
+++ clang/test/Driver/rewrite-legacy-objc.m
@@ -3,11 +3,11 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
 // TEST0: rewrite-legacy-objc.m"
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.9.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST1 %s
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.6.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST2 %s
-// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
-// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
+// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
Index: clang/test/Driver/no-common.c
===
--- /dev/null
+++ clang/test/Driver/no-common.c
@@ -0,0 +1,39 @@
+// RUN: %clang --target=armv7-arm-none-eabi -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang 

[PATCH] D75056: [Driver] Default to -fno-common

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 246457.
SjoerdMeijer retitled this revision from "[ARM][AArch64] Default to 
-fno-common" to "[Driver] Default to -fno-common".
SjoerdMeijer edited the summary of this revision.
SjoerdMeijer added reviewers: tstellar, jyknight.
SjoerdMeijer added a comment.
Herald added subscribers: s.egerton, simoncook, fedor.sergeev, aheejin.

Following the discussion on the llvm dev list, this is a first draft to enable 
`-fno-common` for all targets.

TODO: doc changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75056/new/

https://reviews.llvm.org/D75056

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/2009-10-20-GlobalDebug.c
  clang/test/Driver/no-common.c
  clang/test/Driver/rewrite-legacy-objc.m
  clang/test/Driver/rewrite-objc.m
  clang/test/Frontend/ast-codegen.c

Index: clang/test/Frontend/ast-codegen.c
===
--- clang/test/Frontend/ast-codegen.c
+++ clang/test/Frontend/ast-codegen.c
@@ -5,7 +5,7 @@
 // CHECK: module asm "foo"
 __asm__("foo");
 
-// CHECK: @g0 = common dso_local global i32 0, align 4
+// CHECK: @g0 = dso_local global i32 0, align 4
 int g0;
 
 // CHECK: define dso_local i32 @f0()
Index: clang/test/Driver/rewrite-objc.m
===
--- clang/test/Driver/rewrite-objc.m
+++ clang/test/Driver/rewrite-objc.m
@@ -3,4 +3,4 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
Index: clang/test/Driver/rewrite-legacy-objc.m
===
--- clang/test/Driver/rewrite-legacy-objc.m
+++ clang/test/Driver/rewrite-legacy-objc.m
@@ -3,11 +3,11 @@
 // TEST0: clang{{.*}}" "-cc1"
 // TEST0: "-rewrite-objc"
 // FIXME: CHECK-NOT is broken somehow, it doesn't work here. Check adjacency instead.
-// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST0: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fexceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
 // TEST0: rewrite-legacy-objc.m"
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.9.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST1 %s
 // RUN: %clang -no-canonical-prefixes -target i386-apple-macosx10.6.0 -rewrite-legacy-objc %s -o - -### 2>&1 | \
 // RUN:   FileCheck -check-prefix=TEST2 %s
-// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
-// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fdiagnostics-show-option"
+// TEST1: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"
+// TEST2: "-fmessage-length" "0" "-stack-protector" "1" "-fblocks" "-fencode-extended-block-signature" "-fregister-global-dtors-with-atexit" "-fgnuc-version=4.2.1" "-fobjc-runtime=macosx-fragile" "-fobjc-subscripting-legacy-runtime" "-fno-objc-infer-related-result-type" "-fobjc-exceptions" "-fmax-type-align=16" "-fno-common" "-fdiagnostics-show-option"

[PATCH] D75056: [ARM][AArch64] Default to -fno-common

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
Herald added a subscriber: kristof.beyls.

This patch proposes to default to `-fno-common` for the Arm targets because 
this has performance and code-size benefits.

Additionally, GCC now also defaults to this, so we would be behaving the same 
as GCC:

  commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
  Author: Wilco Dijkstra 
  Date:   Wed Nov 20 16:29:23 2019 +
  PR85678: Change default to -fno-common
  GCC currently defaults to -fcommon.  As discussed in the PR, this is an 
ancient
  C feature which is not conforming with the latest C standards.  On many 
targets
  this means global variable accesses have a codesize and performance 
penalty.
  This applies to C code only, C++ code is not affected by -fcommon.  It is 
about
  time to change the default.


https://reviews.llvm.org/D75056

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/triple-arm-none.c


Index: clang/test/Driver/triple-arm-none.c
===
--- /dev/null
+++ clang/test/Driver/triple-arm-none.c
@@ -0,0 +1,38 @@
+// RUN: %clang --target=armv7-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32LITTLE
+
+// RUN: %clang --target=armv7-arm-none-eabi -mbig-endian -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32BIG
+// RUN: %clang --target=armebv7-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32BIG
+
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -### -c %s 2>&1 | \
+// RUN:  FileCheck %s --check-prefix=CHECK --check-prefix=T32LITTLE
+// RUN: %clang --target=thumbv8a-arm-none-eabi -### -c %s 2>&1  | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -mbig-endian -### -c %s 
2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=T32BIG
+// RUN: %clang --target=thumbv8a-arm-none-eabi -mbig-endian -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+// RUN: %clang --target=thumbebv8a-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+
+// RUN: %clang --target=aarch64_be-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A64BIG
+
+// A32LITTLE: Target: armv7-arm-none-eabi
+// A64LITTLE: Target: aarch64-arm-none-eabi
+// A64BIG: Target: aarch64_be-arm-none-eabi
+
+// CHECK: "-cc1"
+// CHECK-NOT: "-no-integrated-as"
+
+// A32LITTLE: "-triple" "armv7-arm-none-eabi"
+// A32BIG: "-triple" "armebv7-arm-none-eabi"
+// T32LITTLE: "-triple" "thumbv8-arm-none-eabi"
+// T32BIG: "-triple" "thumbebv8-arm-none-eabi"
+// A64LITTLE: "-triple" "aarch64-arm-none-eabi"
+// A64BIG: "-triple" "aarch64_be-arm-none-eabi"
+
+// CHECK: "-fno-common"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1415,6 +1415,12 @@
   return true;
 return false;
 
+  case llvm::Triple::aarch64:
+  case llvm::Triple::aarch64_be:
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
   case llvm::Triple::xcore:
   case llvm::Triple::wasm32:
   case llvm::Triple::wasm64:


Index: clang/test/Driver/triple-arm-none.c
===
--- /dev/null
+++ clang/test/Driver/triple-arm-none.c
@@ -0,0 +1,38 @@
+// RUN: %clang --target=armv7-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32LITTLE
+
+// RUN: %clang --target=armv7-arm-none-eabi -mbig-endian -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32BIG
+// RUN: %clang --target=armebv7-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A32BIG
+
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -### -c %s 2>&1 | \
+// RUN:  FileCheck %s --check-prefix=CHECK --check-prefix=T32LITTLE
+// RUN: %clang --target=thumbv8a-arm-none-eabi -### -c %s 2>&1  | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+
+// RUN: %clang --target=armv8a-arm-none-eabi -mthumb -mbig-endian -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=T32BIG
+// RUN: %clang --target=thumbv8a-arm-none-eabi -mbig-endian -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+// RUN: %clang --target=thumbebv8a-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK
+
+// RUN: %clang --target=aarch64_be-arm-none-eabi -### -c %s 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=CHECK --check-prefix=A64BIG
+
+// A32LITTLE: Target: armv7-arm-none-eabi
+// A64LITTLE: Target: aarch64-arm-none-eabi
+// A64BIG: Target: aarch64_be-arm-none-eabi
+
+// CHECK: "-cc1"
+// CHECK-NOT: "-no-integrated-as"
+
+// A32LITTLE: "-triple" 

[PATCH] D74732: [ARM,CDE] Cosmetic changes, additonal driver tests

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Cheers, LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74732/new/

https://reviews.llvm.org/D74732



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74044: [ARM] Add initial support for Custom Datapath Extension (CDE)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/test/Driver/arm-cde.c:12
+
+// RUN: %clang -target arm-none-none-eabi -march=armv8m.main+cdecp0+cdecp3 %s 
-### -c 2>&1 | FileCheck %s --check-prefix=CHECK-CDE1
+// CHECK-CDE1: "-triple" "thumbv8m.main-none-none-eabi"

Probably good to add a test for v8.1-M too, and also with MVE



Comment at: llvm/lib/Target/ARM/ARMInstrCDE.td:1
+// Immediate operand of arbitrary bit width
+class BitWidthImmOperand

This file needs the copyright header



Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8161
+  case ARM::t2CDP: case ARM::t2CDP2:
+  case ARM::t2LDC2L_OFFSET: case ARM::t2LDC2L_OPTION: case ARM::t2LDC2L_POST: 
case ARM::t2LDC2L_PRE:
+  case ARM::t2LDC2_OFFSET: case ARM::t2LDC2_OPTION: case ARM::t2LDC2_POST: 
case ARM::t2LDC2_PRE:

clang-format this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74044/new/

https://reviews.llvm.org/D74044



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74483: [AArch64] Add Cortex-A34 Support for clang and llvm

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Cheers, LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74483/new/

https://reviews.llvm.org/D74483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74483: [AArch64] Add Cortex-A34 Support for clang and llvm

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Ah, sorry, looks like this is all there is to it, both clang and llvm. It's 
just that a quick grep locally (for a similar core) showed some more results.

Can you (double) check if it needs adding to e.g. a switch in 
`llvm/lib/Support/Host.cpp`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74483/new/

https://reviews.llvm.org/D74483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74483: [AArch64] Add Cortex-A34 Support for clang and llvm

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks like the usual business of adding a cpu to me, with one nit inlined that 
can be fixed before committing.

Looks like you're doing the LLVM part separately. Now with the monorepo you 
could have conveniently done it in one patch, but nothing wrong with doing it 
separately.  You could have linked to the llvm part here if that is ready, 
which would be nice to get an overview.




Comment at: llvm/unittests/Support/TargetParserTest.cpp:784
   EXPECT_TRUE(testAArch64CPU(
+  "cortex-a34", "armv8-a", "crypto-neon-fp-armv8",
+  AArch64::AEK_CRC | AArch64::AEK_SIMD |

nit: this looks the same as the a35. Would be better to keep the same order of 
arch extension and also the formatting (to make it easier to eyeball 
differences).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74483/new/

https://reviews.llvm.org/D74483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57054: [MachineOutliner][ARM][RFC] Add Machine Outliner support for ARM

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a subscriber: samparker.
SjoerdMeijer added a comment.

Just a quick message, linking in @samparker, and I guess moving Low Overhead 
Loops to run before ConstantIslands could be problematic, but we can/should 
have a proper look tomorrow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57054/new/

https://reviews.llvm.org/D57054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG021685491727: [Clang] Pragma vectorize_width() implies 
vectorize(enable) (authored by SjoerdMeijer).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -161,20 +161,20 @@
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +185,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -207,11 +207,11 @@
 // CHECK: ![[AFTER_VECTOR_12]] = distinct !{![[AFTER_VECTOR_12]], ![[ISVECTORIZED:.*]], ![[UNROLL_24:.*]]}
 // CHECK: ![[UNROLL_24]] = !{!"llvm.loop.unroll.count", i32 24}
 
-// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[FOLLOWUP_VECTOR_13:.*]]}
+// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_13:.*]]}
 // CHECK: ![[INTERLEAVE_16]] = !{!"llvm.loop.interleave.count", i32 16}
 // CHECK: ![[FOLLOWUP_VECTOR_13]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_13:.*]]}
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
Index: clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+void loop1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP1:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Here, vectorize.enable should be set, obviously, but also check that
+// metadata isn't added twice.
+void loop2(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop2{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP2:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(2)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Test that we do *not* imply vectorize.enable.
+void 

[Diffusion] rG825235c140e7: Revert "[Sema] Use the canonical type in function isVector"

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I don't think I have full context of the problem but in this diff I see 
type `__fp16`, which is the storage-only type. But if we are talking about 
v8.2-A and FP16, then we are talking about the native type (e.g. 
source-language type `_Float16`), and for that you need to enable the `+fp16` 
architecture extension.  I would have to refresh my memory, but I thought 
`-fnative-half-type` was used to enable OpenCL fp16 types? Either way, it looks 
like there's interaction between `__fp16` and native FP16 types, but I don't 
think adding `-fnative-half-type` is the right approach. But as I said, I don't 
know exactly what the problem is, and would need to have a proper look when I'm 
back at my desk.


BRANCHES
  master

Users:
  ahatanak (Author)

https://reviews.llvm.org/rG825235c140e7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.



Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:302-306
+// Imply vectorize.enable when it is not already disabled/enabled.
+Args.push_back(
+MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+  ConstantAsMetadata::get(ConstantInt::get(
+  llvm::Type::getInt1Ty(Ctx), 1))}));

Meinersbur wrote:
> SjoerdMeijer wrote:
> > Meinersbur wrote:
> > > [serious] Why not reusing the `Args.push_back` code from above? I think 
> > > it is important `vectorize_predicate` and `vectorize_width` (and ever 
> > > additional property we introduce in the future) the same way. IMHO 
> > > everything else becomes more and more confusing.
> > > I have the following in mind:
> > > 
> > > ```
> > > if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
> > >   IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1) {
> > > auto AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
> > > Args.push_back(..., ConstantInt::get(AttrVal));
> > > }
> > > ```
> > > 
> > No worries, and thanks for looking again! I was a bit reluctant to touch 
> > that piece of logic (and actually thought this if-elseif was not too bad 
> > and explicit in identifying the different cases), but yeah, what you 
> > suggest make sense, so will address this soon.
> Sounds like a typical case of "[[ 
> https://en.wikipedia.org/wiki/Technical_debt | technical debt ]]".
yep, but it's been addressed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 229255.
SjoerdMeijer added a comment.

Thanks again for the suggestion; this indeed (hopefully) looks a lot neater now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -161,20 +161,20 @@
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +185,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -207,11 +207,11 @@
 // CHECK: ![[AFTER_VECTOR_12]] = distinct !{![[AFTER_VECTOR_12]], ![[ISVECTORIZED:.*]], ![[UNROLL_24:.*]]}
 // CHECK: ![[UNROLL_24]] = !{!"llvm.loop.unroll.count", i32 24}
 
-// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[FOLLOWUP_VECTOR_13:.*]]}
+// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_13:.*]]}
 // CHECK: ![[INTERLEAVE_16]] = !{!"llvm.loop.interleave.count", i32 16}
 // CHECK: ![[FOLLOWUP_VECTOR_13]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_13:.*]]}
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
Index: clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+void loop1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP1:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Here, vectorize.enable should be set, obviously, but also check that
+// metadata isn't added twice.
+void loop2(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop2{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP2:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(2)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Test that we do *not* imply vectorize.enable.
+void loop3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop3{{.*}}(
+// CHECK: br label {{.*}}, 

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.



Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:302-306
+// Imply vectorize.enable when it is not already disabled/enabled.
+Args.push_back(
+MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+  ConstantAsMetadata::get(ConstantInt::get(
+  llvm::Type::getInt1Ty(Ctx), 1))}));

Meinersbur wrote:
> [serious] Why not reusing the `Args.push_back` code from above? I think it is 
> important `vectorize_predicate` and `vectorize_width` (and ever additional 
> property we introduce in the future) the same way. IMHO everything else 
> becomes more and more confusing.
> I have the following in mind:
> 
> ```
> if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
>   IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1) {
> auto AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
> Args.push_back(..., ConstantInt::get(AttrVal));
> }
> ```
> 
No worries, and thanks for looking again! I was a bit reluctant to touch that 
piece of logic (and actually thought this if-elseif was not too bad and 
explicit in identifying the different cases), but yeah, what you suggest make 
sense, so will address this soon.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 227273.
SjoerdMeijer added a comment.

Thanks Michael!

- moved the handling of vectorize.enable to 1 place,
- that should have also sorted the relative ordering, and duplication of the 
metadata in some cases,
- created a separate file for the new tests, and added some more tests (also 
making sure there are no duplicates).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -161,20 +161,20 @@
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +185,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -207,11 +207,11 @@
 // CHECK: ![[AFTER_VECTOR_12]] = distinct !{![[AFTER_VECTOR_12]], ![[ISVECTORIZED:.*]], ![[UNROLL_24:.*]]}
 // CHECK: ![[UNROLL_24]] = !{!"llvm.loop.unroll.count", i32 24}
 
-// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[FOLLOWUP_VECTOR_13:.*]]}
+// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], ![[INTERLEAVE_16:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_13:.*]]}
 // CHECK: ![[INTERLEAVE_16]] = !{!"llvm.loop.interleave.count", i32 16}
 // CHECK: ![[FOLLOWUP_VECTOR_13]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_13:.*]]}
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]], ![[VECTORIZE_ENABLE]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
Index: clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pragma-loop-pr27643.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+void loop1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP1:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Here, vectorize.enable should be set, obviously, but also check that
+// metadata isn't added twice.
+void loop2(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}loop2{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP2:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(2)
+  for (int i 

[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added reviewers: Meinersbur, fhahn, rupprecht.

This got reverted because given the following source:

  void a() {
#pragma clang loop vectorize(disable)
for (;;)
  ;
  }


it incorrectly enabled vectorisation and set metadata due to a logic error. 
With this fixed, we now imply vectorisation when:

1. vectorisation is enabled, which means: VectorizeWidth > 1
2. and don't want to add it when it is disabled or enabled, otherwise we would 
be incorrectly setting it or duplicating the metadata, respectively.


https://reviews.llvm.org/D69628

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,23 +158,41 @@
   for_template_constant_expression_test(List, Length);
 }
 
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+void width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +203,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[VECTORIZE_ENABLE]], ![[WIDTH_5:.*]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -213,5 +231,9 @@
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[VECTORIZE_ENABLE]], ![[WIDTH_10:.*]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK:  ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[WIDTH_1]], ![[VECTORIZE_ENABLE]]}
+
+// CHECK-NEXT: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[WIDTH_1]]}
Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,7 +58,6 @@
 List[i] = i * 2;
 }
 
-
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
 
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -268,6 +268,14 

[PATCH] D68838: [ARM] Fix arm_neon.h with -flax-vector-conversions=none, part 3

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Yep, thanks again!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68838/new/

https://reviews.llvm.org/D68838



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66199: [docs] loop pragmas

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG52bfa73af841: [docs] loop pragmas: options implying 
transformations (authored by SjoerdMeijer).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66199/new/

https://reviews.llvm.org/D66199

Files:
  clang/docs/LanguageExtensions.rst


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -3027,6 +3027,14 @@
 distribution. Loop hints can be specified before any loop and will be ignored 
if
 the optimization is not safe to apply.
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there are loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options
+imply the transformation is enabled, as if it was enabled via the corresponding
+transformation pragma (e.g. ``vectorize(enable)``). If the transformation is
+disabled  (e.g. ``vectorize(disable)``), that takes precedence over
+transformations option pragmas implying that transformation.
+
 Vectorization, Interleaving, and Predication
 
 


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -3027,6 +3027,14 @@
 distribution. Loop hints can be specified before any loop and will be ignored if
 the optimization is not safe to apply.
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there are loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options
+imply the transformation is enabled, as if it was enabled via the corresponding
+transformation pragma (e.g. ``vectorize(enable)``). If the transformation is
+disabled  (e.g. ``vectorize(disable)``), that takes precedence over
+transformations option pragmas implying that transformation.
+
 Vectorization, Interleaving, and Predication
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/lib/Basic/Targets/ARM.cpp:902
+  std::vector  = getTargetOpts().Features;
+  std::string SearchFeature = "+reserve-" + RegName.str();
+  for (std::string  : Features) {

chill wrote:
> SjoerdMeijer wrote:
> > I was pointed at something similar myself recently, but if I am not 
> > mistaken then I think this is a use-after-free:
> > 
> >"+reserve-" + RegName.str()
> > 
> > this will allocate a temp `std::string` that `SearchFeature` points to, 
> > which then gets released, and `SearchFeature` is still pointing at it.
> Any temporaries would be destructed at the end of the full expression. By 
> that time, the `SearchString` would be constructed and stand on its own.
Ah yes, true. This is a std::string, not stringref as in my case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68862/new/

https://reviews.llvm.org/D68862



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68862: [ARM] Allocatable Global Register Variables for ARM

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Bit of a drive-by comment, but I can't say I am big fan of all the string 
matching on the register names. Not sure if this is a fair comment, because I 
haven't looked closely at it yet, but could we use more the `ARM::R[0-9]` 
values more? Perhaps that's difficult from the Clang parts?




Comment at: clang/lib/Basic/Targets/ARM.cpp:902
+  std::vector  = getTargetOpts().Features;
+  std::string SearchFeature = "+reserve-" + RegName.str();
+  for (std::string  : Features) {

I was pointed at something similar myself recently, but if I am not mistaken 
then I think this is a use-after-free:

   "+reserve-" + RegName.str()

this will allocate a temp `std::string` that `SearchFeature` points to, which 
then gets released, and `SearchFeature` is still pointing at it.



Comment at: llvm/test/CodeGen/ARM/reg-alloc-with-fixed-reg-r6-modified.ll:15
+; r6 = 10;
+; unsigned int result = i + j + k + l +m + n + o + p;
+; }

nit: `+m` -> ` + m`



Comment at: llvm/test/CodeGen/ARM/reg-alloc-with-fixed-reg-r6.ll:13
+; {
+; unsigned int result = i + j + k + l +m + n + o + p;
+; }

same nit



Comment at: llvm/test/CodeGen/ARM/reg-alloc-wout-fixed-regs.ll:3
+;
+; Equivalent C source code
+; void bar(unsigned int i,

As all these tests (this file and the ones above) are the same, the "equivalent 
C source code" is the same, perhaps move all these cases into 1 file.



Comment at: llvm/test/CodeGen/ARM/reg-alloc-wout-fixed-regs.ll:13
+; {
+; unsigned int result = i + j + k + l +m + n + o + p;
+; }

same nit here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68862/new/

https://reviews.llvm.org/D68862



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66199: [docs] loop pragmas

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Sure, will do, thanks again for taking a look.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66199/new/

https://reviews.llvm.org/D66199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66199: [docs] loop pragmas

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 224421.
SjoerdMeijer added a comment.

Thanks! Typo fixed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66199/new/

https://reviews.llvm.org/D66199

Files:
  clang/docs/LanguageExtensions.rst


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -3027,6 +3027,14 @@
 distribution. Loop hints can be specified before any loop and will be ignored 
if
 the optimization is not safe to apply.
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there are loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options
+imply the transformation is enabled, as if it was enabled via the corresponding
+transformation pragma (e.g. ``vectorize(enable)``). If the transformation is
+disabled  (e.g. ``vectorize(disable)``), that takes precedence over
+transformations option pragmas implying that transformation.
+
 Vectorization, Interleaving, and Predication
 
 


Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -3027,6 +3027,14 @@
 distribution. Loop hints can be specified before any loop and will be ignored if
 the optimization is not safe to apply.
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there are loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options
+imply the transformation is enabled, as if it was enabled via the corresponding
+transformation pragma (e.g. ``vectorize(enable)``). If the transformation is
+disabled  (e.g. ``vectorize(disable)``), that takes precedence over
+transformations option pragmas implying that transformation.
+
 Vectorization, Interleaving, and Predication
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66199: [docs] loop pragmas

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I have commit all my pragma patches, so now back to the last bit, this doc 
update. 
This doc change should now reflect our implementation. Are we happy for this to 
go in?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66199/new/

https://reviews.llvm.org/D66199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68743: [ARM] Fix arm_neon.h with -flax-vector-conversions=none, part 2.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Thanks again, looks good.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68743/new/

https://reviews.llvm.org/D68743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68683: ARM] Fix arm_neon.h with -flax-vector-conversions=none

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Nice one, thanks for fixing! I didn't have the bandwidth to look into this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68683/new/

https://reviews.llvm.org/D68683



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Many thanks! I've passed on the message, hopefully we can do something about 
this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61717/new/

https://reviews.llvm.org/D61717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372082: [Clang] Pragma vectorize_width() implies 
vectorize(enable) (authored by SjoerdMeijer, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66290?vs=220446=220448#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290

Files:
  cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop.cpp

Index: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
@@ -270,6 +270,14 @@
 
   // Setting vectorize.width
   if (Attrs.VectorizeWidth > 0) {
+// This implies vectorize.enable = true, but only add it when it is not
+// already enabled.
+if (Attrs.VectorizeEnable != LoopAttributes::Enable)
+  Args.push_back(
+  MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+ConstantAsMetadata::get(ConstantInt::get(
+llvm::Type::getInt1Ty(Ctx), 1))}));
+
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.width"),
 ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt32Ty(Ctx),
Index: cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,7 +58,6 @@
 List[i] = i * 2;
 }
 
-
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
 
@@ -70,7 +69,7 @@
 
 // CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
 
-// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
+// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !3, !10}
 // CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
 
-// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !3, !10}
Index: cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
@@ -158,23 +158,41 @@
   for_template_constant_expression_test(List, Length);
 }
 
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+void width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +203,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct 

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 220446.
SjoerdMeijer added a comment.

Just uploading new diff for completeness; I only had to change a test-case, and 
thus thought that committing this is okay.

Many thanks again for reviewing and helping with the discussions!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,23 +158,41 @@
   for_template_constant_expression_test(List, Length);
 }
 
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+void width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +203,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[VECTORIZE_ENABLE]], ![[WIDTH_5:.*]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -213,5 +231,9 @@
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[VECTORIZE_ENABLE]], ![[WIDTH_10:.*]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK:  ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[WIDTH_1]], ![[VECTORIZE_ENABLE]]}
+
+// CHECK-NEXT: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[VECTORIZE_ENABLE]], ![[WIDTH_1]]}
Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -58,7 +58,6 @@
 List[i] = i * 2;
 }
 
-
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
 
@@ -70,7 +69,7 @@
 
 // CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
 
-// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
+// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !3, !10}
 // CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
 
-// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !3, !10}
Index: 

[PATCH] D67368: [NFCI]Create CommonAttributeInfo Type as base type of *Attr and ParsedAttr.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I would have definitely preferred a revert much earlier, I guess it's too late 
now, but not having a build is really inconvenient.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67368/new/

https://reviews.llvm.org/D67368



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Yeah, sorry about that

Do you perhaps have a test case or error that I can look at? Perhaps I or 
someone else here can help out a bit here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61717/new/

https://reviews.llvm.org/D61717



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer abandoned this revision.
SjoerdMeijer added a comment.

Every day is a school day, and Hal taught me something ;-)

As I wrote in the thread on the llvm dev list, with Hal's explanations, I don't 
think I have a case for this anymore, and so will abandon it (please let me 
know if you disagree).

Many thanks for reviewing and the discussions!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Now on the practical side. If whatever we decide here changes how the pragma 
> behaves from today, we need to have a wider discussion and agreement at 
> llvm-dev.

Yep, forgot about that, thanks for the suggestion. I've just posted this 
message to the list:

http://lists.llvm.org/pipermail/llvm-dev/2019-September/135016.html


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

And thanks Florian for getting this discussion going again! Will definitely 
make this clear(er) in this description and commit message once we agree on it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Hi Hideki,

I think you're comments are spot on:

> It all depends on to whom we are providing these pragmas.

Pragma's are user-facing "options" to override or force  compiler decision 
making. I don't think there's another way to look at it, but please correct me 
if I'm wrong.

That's exactly the reason why I think `vectorize(disable)` should disable 
vectorisation for that loop. I just don't see what else a user would expect.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

friendly ping.

Shall we get this in, so that I can commit this and  D66290 
? Then, we can perhaps continue the interleave 
discussion separately? 
I will then return to the doc patch first in D66199 
, so that we can start wrapping up the loop 
pragma business, and hopefully it is a lot more consistent!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Therefore, vectorize(disable) would also disable interleaving?

I don't have strong opinions on this. I think there's something to say for both 
options that we have (i.e. `vectorize(disable)` disables interleaving or 
enables it).
But I think it was @fhahn who mentioned that it would be possible to disable 
vectorisation, but still do interleaving. Perhaps bit of a edge use use, but 
again, it's possible currently. That would mean we don't need to disable 
interleaving here I think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66199: [docs] loop pragmas

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Many thanks for your clarification!

>   What we were discussing was that these settings would remove 0) from the 
> candidate list as well.

Yep, that's crystal clear now.
And my expectation would indeed be that this would be the case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66199/new/

https://reviews.llvm.org/D66199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 217383.
SjoerdMeijer edited the summary of this revision.
SjoerdMeijer added a parent revision: D66796: [clang] Loop pragma 
vectorize(disable).
SjoerdMeijer added a comment.

Stripped out the functional change related to vectorize(disable).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp


Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,23 +158,41 @@
   for_template_constant_expression_test(List, Length);
 }
 
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+void width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], 
![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], 
![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], 
![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], 
![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
@@ -185,7 +203,7 @@
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[VECTORIZE_ENABLE]], 
![[WIDTH_5:.*]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -213,5 +231,9 @@
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], 
![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[VECTORIZE_ENABLE]], 
![[WIDTH_10:.*]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK:  ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[VEC_WIDTH_1:.*]], 
![[VECTORIZE_ENABLE]]}
+// CHECK-NEXT: ![[VEC_WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK-NEXT: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[VECTORIZE_ENABLE]], 
![[VEC_WIDTH_1]]}
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -270,6 +270,14 @@
 
   // Setting vectorize.width
   if (Attrs.VectorizeWidth > 0) {
+// which implies vectorize.enable = true, but only add it when it is not
+// already enabled.
+if (Attrs.VectorizeEnable != LoopAttributes::Enable)
+  Args.push_back(
+  MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+ConstantAsMetadata::get(ConstantInt::get(
+llvm::Type::getInt1Ty(Ctx), 1))}));
+
 Metadata *Vals[] = {
 MDString::get(Ctx, 

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> I think it would be slightly better to split off the change to disable 
> vectorization via llvm.loop.vectorize.enable=false instead of width=1.

This is now D66796 .

I will now start stripping it out from this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added reviewers: fhahn, Meinersbur, hsaito, Ayal.

This changes the behaviour of vectorize(disable). I.e., disabling vectorization
now means disabling vectorization, and not setting the vectorization width to 1.

This is a follow up of the discussion on the behaviour of different loop
pragmas, and that setting transformation options should imply enabling the
transformation, see also
http://lists.llvm.org/pipermail/cfe-dev/2019-August/063054.html.

Not only is this change in the behaviour of vectorize(disable) probably more 
sensible,
it also helps in implementing options implying transformations.


https://reviews.llvm.org/D66796

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp
  clang/test/CodeGenCXX/pragma-loop-safety.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp


Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -178,8 +178,8 @@
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
-// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_6:.*]]}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-safety.cpp
+++ clang/test/CodeGenCXX/pragma-loop-safety.cpp
@@ -53,6 +53,6 @@
 // CHECK: ![[INTERLEAVE_1]] = !{!"llvm.loop.interleave.count", i32 1}
 // CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[ACCESS_GROUP_8]] = distinct !{}
-// CHECK: ![[LOOP2_HINTS]] = distinct !{![[LOOP2_HINTS]], 
![[PARALLEL_ACCESSES_11:[0-9]+]], ![[UNROLL_DISABLE]], ![[WIDTH_1:[0-9]+]], 
![[INTENABLE_1]]}
+// CHECK: ![[LOOP2_HINTS]] = distinct !{![[LOOP2_HINTS]], 
![[PARALLEL_ACCESSES_11:[0-9]+]], ![[UNROLL_DISABLE]], 
![[VECTORIZE_DISABLE:[0-9]+]]}
 // CHECK: ![[PARALLEL_ACCESSES_11]] = !{!"llvm.loop.parallel_accesses", 
![[ACCESS_GROUP_8]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -71,6 +71,6 @@
 // CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
 
 // CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
-// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -617,8 +617,7 @@
 case LoopHintAttr::Disable:
   switch (Option) {
   case LoopHintAttr::Vectorize:
-// Disable vectorization by specifying a width of 1.
-setVectorizeWidth(1);
+setVectorizeEnable(false);
 break;
   case LoopHintAttr::Interleave:
 // Disable interleaving by speciyfing a count of 1.


Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -178,8 +178,8 @@
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
-// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- 

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks, and sorry for the delay. Back in the office now, and I am addressing 
this:

> I think it would be slightly better to split off the change to disable 
> vectorization via llvm.loop.vectorize.enable=false instead of width=1.

Yep, I agree


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for looking again!
Good catch, feedback addressed.

(forgot to add this message when I uploaded the new diff)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 215388.
SjoerdMeijer edited the summary of this revision.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66290/new/

https://reviews.llvm.org/D66290

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp
  clang/test/CodeGenCXX/pragma-loop-safety.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -158,34 +158,53 @@
   for_template_constant_expression_test(List, Length);
 }
 
+void vec_width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}vec_width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+
+  #pragma clang loop vectorize(enable) vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+void width_1(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}width_1{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+
+  #pragma clang loop vectorize_width(1)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
-// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[VECTORIZE_ENABLE]], ![[WIDTH_5:.*]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -213,5 +232,9 @@
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[VECTORIZE_ENABLE]], ![[WIDTH_10:.*]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+
+// CHECK:  ![[LOOP_15]] = distinct !{![[LOOP_15]], ![[VEC_WIDTH_1:.*]], ![[VECTORIZE_ENABLE]]}
+// CHECK-NEXT: ![[VEC_WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK-NEXT: ![[LOOP_16]] = distinct !{![[LOOP_16]], ![[VECTORIZE_ENABLE]], ![[VEC_WIDTH_1]]}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-safety.cpp
+++ clang/test/CodeGenCXX/pragma-loop-safety.cpp
@@ -53,6 +53,6 @@
 // CHECK: ![[INTERLEAVE_1]] = !{!"llvm.loop.interleave.count", i32 1}
 // CHECK: ![[INTENABLE_1]] = 

[PATCH] D66290: [clang] Pragma vectorize_width() implies vectorize(enable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added reviewers: Meinersbur, fhahn, hsaito, dorit.

Specifying the vectorization width was supposed to implicitly enable
vectorization, except that it wasn't really doing this. It was only
setting the `vectorize.width` metadata, but not `vectorize.enable`.

 

And related to this, vectorize(disable) was translated to
 `vectorize_width(1)`, but now this simply translates to vectorize.enable = 
false.

 

As also pointed out in the discussion on the cfe dev list, this is probably a 
bit 
of a silly combination:

 
  vectorize(enable) vectorize_width(1)
 

but it could still mean that the vectorizer interleaves. So, with this
simplification, disabled means disabled, and a width of 1 a width of 1.

 

This should also fix PR27643.


https://reviews.llvm.org/D66290

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp
  clang/test/CodeGenCXX/pragma-loop-safety.cpp
  clang/test/CodeGenCXX/pragma-loop.cpp

Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -161,31 +161,31 @@
 // CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
 
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_ENABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
+// CHECK: ![[VECTORIZE_ENABLE]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
 
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[VECTORIZE_ENABLE]], ![[FOLLOWUP_VECTOR_3:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
 // CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 // CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
 
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
+// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[VECTORIZE_ENABLE]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
 
-// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
 // CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_6:.*]]}
 // CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
 
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[VECTORIZE_ENABLE]], ![[WIDTH_5:.*]]}
 // CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
 
 // CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
@@ -213,5 +213,5 @@
 // CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], ![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
 // CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
 
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[VECTORIZE_ENABLE]], ![[WIDTH_10:.*]]}
 // CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-safety.cpp
+++ clang/test/CodeGenCXX/pragma-loop-safety.cpp
@@ -53,6 +53,6 @@
 // CHECK: ![[INTERLEAVE_1]] = !{!"llvm.loop.interleave.count", i32 1}
 // CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[ACCESS_GROUP_8]] = distinct !{}
-// CHECK: ![[LOOP2_HINTS]] = distinct !{![[LOOP2_HINTS]], ![[PARALLEL_ACCESSES_11:[0-9]+]], ![[UNROLL_DISABLE]], ![[WIDTH_1:[0-9]+]], 

[PATCH] D64564: Loop pragma parsing. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368976: [clang] Loop pragma parsing. NFC. (authored by 
SjoerdMeijer, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64564?vs=215162=215336#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64564/new/

https://reviews.llvm.org/D64564

Files:
  cfe/trunk/lib/Parse/ParsePragma.cpp


Index: cfe/trunk/lib/Parse/ParsePragma.cpp
===
--- cfe/trunk/lib/Parse/ParsePragma.cpp
+++ cfe/trunk/lib/Parse/ParsePragma.cpp
@@ -1006,18 +1006,13 @@
 } // end anonymous namespace
 
 static std::string PragmaLoopHintString(Token PragmaName, Token Option) {
-  std::string PragmaString;
-  if (PragmaName.getIdentifierInfo()->getName() == "loop") {
-PragmaString = "clang loop ";
-PragmaString += Option.getIdentifierInfo()->getName();
-  } else if (PragmaName.getIdentifierInfo()->getName() == "unroll_and_jam") {
-PragmaString = "unroll_and_jam";
-  } else {
-assert(PragmaName.getIdentifierInfo()->getName() == "unroll" &&
-   "Unexpected pragma name");
-PragmaString = "unroll";
-  }
-  return PragmaString;
+  StringRef Str = PragmaName.getIdentifierInfo()->getName();
+  std::string ClangLoopStr = (llvm::Twine("clang loop ") + Str).str();
+  return llvm::StringSwitch(Str)
+  .Case("loop", ClangLoopStr)
+  .Case("unroll_and_jam", Str)
+  .Case("unroll", Str)
+  .Default("");
 }
 
 bool Parser::HandlePragmaLoopHint(LoopHint ) {
@@ -1041,12 +1036,12 @@
 
   // Return a valid hint if pragma unroll or nounroll were specified
   // without an argument.
-  bool PragmaUnroll = PragmaNameInfo->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameInfo->getName() == "nounroll";
-  bool PragmaUnrollAndJam = PragmaNameInfo->getName() == "unroll_and_jam";
-  bool PragmaNoUnrollAndJam = PragmaNameInfo->getName() == "nounroll_and_jam";
-  if (Toks.empty() && (PragmaUnroll || PragmaNoUnroll || PragmaUnrollAndJam ||
-   PragmaNoUnrollAndJam)) {
+  auto IsLoopHint = llvm::StringSwitch(PragmaNameInfo->getName())
+.Cases("unroll", "nounroll", "unroll_and_jam",
+   "nounroll_and_jam", true)
+.Default(false);
+
+  if (Toks.empty() && IsLoopHint) {
 ConsumeAnnotationToken();
 Hint.Range = Info->PragmaName.getLocation();
 return true;


Index: cfe/trunk/lib/Parse/ParsePragma.cpp
===
--- cfe/trunk/lib/Parse/ParsePragma.cpp
+++ cfe/trunk/lib/Parse/ParsePragma.cpp
@@ -1006,18 +1006,13 @@
 } // end anonymous namespace
 
 static std::string PragmaLoopHintString(Token PragmaName, Token Option) {
-  std::string PragmaString;
-  if (PragmaName.getIdentifierInfo()->getName() == "loop") {
-PragmaString = "clang loop ";
-PragmaString += Option.getIdentifierInfo()->getName();
-  } else if (PragmaName.getIdentifierInfo()->getName() == "unroll_and_jam") {
-PragmaString = "unroll_and_jam";
-  } else {
-assert(PragmaName.getIdentifierInfo()->getName() == "unroll" &&
-   "Unexpected pragma name");
-PragmaString = "unroll";
-  }
-  return PragmaString;
+  StringRef Str = PragmaName.getIdentifierInfo()->getName();
+  std::string ClangLoopStr = (llvm::Twine("clang loop ") + Str).str();
+  return llvm::StringSwitch(Str)
+  .Case("loop", ClangLoopStr)
+  .Case("unroll_and_jam", Str)
+  .Case("unroll", Str)
+  .Default("");
 }
 
 bool Parser::HandlePragmaLoopHint(LoopHint ) {
@@ -1041,12 +1036,12 @@
 
   // Return a valid hint if pragma unroll or nounroll were specified
   // without an argument.
-  bool PragmaUnroll = PragmaNameInfo->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameInfo->getName() == "nounroll";
-  bool PragmaUnrollAndJam = PragmaNameInfo->getName() == "unroll_and_jam";
-  bool PragmaNoUnrollAndJam = PragmaNameInfo->getName() == "nounroll_and_jam";
-  if (Toks.empty() && (PragmaUnroll || PragmaNoUnroll || PragmaUnrollAndJam ||
-   PragmaNoUnrollAndJam)) {
+  auto IsLoopHint = llvm::StringSwitch(PragmaNameInfo->getName())
+.Cases("unroll", "nounroll", "unroll_and_jam",
+   "nounroll_and_jam", true)
+.Default(false);
+
+  if (Toks.empty() && IsLoopHint) {
 ConsumeAnnotationToken();
 Hint.Range = Info->PragmaName.getLocation();
 return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64564: Loop pragma parsing. NFC.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks, and will fix your suggestions before committing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64564/new/

https://reviews.llvm.org/D64564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65776: [Clang] Pragma vectorize_predicate implies vectorize

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368970: [Clang] Pragma vectorize_predicate implies vectorize 
(authored by SjoerdMeijer, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65776?vs=213462=215330#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65776/new/

https://reviews.llvm.org/D65776

Files:
  cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp


Index: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
@@ -253,12 +253,18 @@
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
   // Setting vectorize.predicate
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
+  bool IsVectorPredicateEnabled = false;
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
+  Attrs.VectorizeEnable != LoopAttributes::Disable &&
+  Attrs.VectorizeWidth < 1) {
+
+IsVectorPredicateEnabled =
+(Attrs.VectorizePredicateEnable == LoopAttributes::Enable);
+
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.predicate.enable"),
-ConstantAsMetadata::get(ConstantInt::get(
-llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizePredicateEnable == LoopAttributes::Enable)))};
+ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt1Ty(Ctx),
+ IsVectorPredicateEnabled))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
@@ -281,12 +287,15 @@
   }
 
   // Setting vectorize.enable
-  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified) {
+  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
+  IsVectorPredicateEnabled) {
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.enable"),
 ConstantAsMetadata::get(ConstantInt::get(
 llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizeEnable == LoopAttributes::Enable)))};
+IsVectorPredicateEnabled
+? true
+: (Attrs.VectorizeEnable == LoopAttributes::Enable)))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
Index: cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -27,9 +27,50 @@
 List[i] = i * 2;
 }
 
+// vectorize_predicate(enable) implies vectorize(enable)
+void test3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test3{{.*}}(
+// CHECK:   br label {{.*}}, !llvm.loop ![[LOOP3:.*]]
+
+  #pragma clang loop vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that disabling vectorization means a vectorization width of 1, and
+// also that vectorization_predicate isn't enabled.
+void test4(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test4{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP4:.*]]
+
+  #pragma clang loop vectorize(disable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that vectorize and vectorize_predicate are disabled.
+void test5(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test5{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP5:.*]]
+
+  #pragma clang loop vectorize(disable) vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP1]] = distinct !{![[LOOP1]], !5, !3}
 // CHECK-NEXT: !5 = !{!"llvm.loop.vectorize.predicate.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP2]] = distinct !{![[LOOP2]], !7, !3}
 // CHECK-NEXT: !7 = !{!"llvm.loop.vectorize.predicate.enable", i1 false}
+
+// CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
+
+// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
+// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
+
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}


Index: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
@@ -253,12 +253,18 @@
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
   // Setting vectorize.predicate
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
+  bool IsVectorPredicateEnabled = false;
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
+  Attrs.VectorizeEnable != LoopAttributes::Disable &&
+  Attrs.VectorizeWidth < 1) {
+
+IsVectorPredicateEnabled =
+(Attrs.VectorizePredicateEnable ==