[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4653669 , @lei wrote:

> HI @nemanjai, Did you get a chance to post this as a github PR?

Long overdue, but I finally have it up at 
https://github.com/llvm/llvm-project/pull/68919


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Lei Huang via Phabricator via cfe-commits]

lei added a comment.

HI @nemanjai, Did you get a chance to post this as a github PR?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4602914 , @ilinpv wrote:

> Friendly ping, are there any questions remained to proceed with 
> target-independent __builtin_cpu_supports ?

Sorry, I have not gotten back to this review in quite some time as I have been 
relocating to a different country. I'll try to post an updated version of this 
on GitHub this weekend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv added a comment.

Friendly ping, are there any questions remained to proceed with 
target-independent __builtin_cpu_supports ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

nemanjai wrote:
> arsenm wrote:
> > nemanjai wrote:
> > > arsenm wrote:
> > > > nemanjai wrote:
> > > > > arsenm wrote:
> > > > > > From this description I don't understand what this is supposed to 
> > > > > > do. What does the input mean? Why does this use an i32 immarg and 
> > > > > > not a pointer? Why is the result only i32?
> > > > > That is fair enough. The description is fairly vague. I can try to 
> > > > > improve it as per below. The parameter for this is not a pointer 
> > > > > (i.e. not an address). It is an immediate that represents the index 
> > > > > into the enumeration of values that are provided at a fixed address. 
> > > > > The back end is then free to produce the actual fixed address and the 
> > > > > load itself.
> > > > > The choice for the result type was admittedly arbitrary - on PPC, the 
> > > > > values provided by GLIBC are 32-bit words.
> > > > > 
> > > > > Proposed comment describing this intrinsic:
> > > > > ```
> > > > > // This intrinsic is provided to allow back ends to emit load
> > > > > // instructions that load a value from a fixed address. The
> > > > > // parameter to the intrinsic is not an address, but an
> > > > > // immediate index into an enumeration that contains the
> > > > > // union of all such values available on all back ends.
> > > > > // An example is the HWCAP/HWCAP2/CPUID words
> > > > > // provided by GLIBC on PowerPC to allow fast access
> > > > > // to commonly used parts of AUXV. These are provided
> > > > > // at a fixed offset into the TCB (accessible through the
> > > > > // thread pointer).
> > > > > ```
> > > > This is baking an a very target specific implementation of device 
> > > > identification. Could you redefine this as something more abstract? 
> > > > Like returns a device ID integer, or a bool that some int input is 
> > > > supported?
> > > The idea is for this to not be restricted to device ID at all, but to be 
> > > used for any values that reside in a fixed address for the compiler. For 
> > > example, `STACK_GUARD` can be one of the values. How about if the 
> > > intrinsic returns any integer type (or maybe any type)?
> > > My thinking is that there may be need in the future for various things to 
> > > be loaded from fixed addresses. The list of possible things that can be 
> > > loaded this way would be a union of what all targets want and only 
> > > specific values would make sense on each target.
> > But you're assuming there's a fixed address this can be loaded from, and 
> > not a read from a special register or some other mechanism
> Oh, well sure. My intent was just for those things that are in memory at a 
> fixed address. But I suppose you're right, there is probably not a 
> fundamental reason to restrict it to things at fixed addresses.
> 
> What I am trying to avoid here is defining an intrinsic that takes a specific 
> value and returns a `bool` (i.e. the intrinsic version of 
> `__builtin_cpu_{supports|is}`. The features/cpus will be different for every 
> target and mapping features to integers is also target specific. So this was 
> an attempt to implement an intrinsic that gets a value (bit vector if you 
> will) that the target can choose how to lower and what to do with the value 
> (i.e. how to mask it).
> 
> I suppose each target can define their own intrinsics for accessing CPU 
> identification information since each target has to provide code generation 
> for them anyway.
I would not restrict targets to specific instrinsics for feature detection. 
Please notice that currently CPU identification is more complicated than 
HWCAPs, including reads from system registers. It is done in compiler-rt 
library (**cpu_model.c**), in **init_cpu_features_resolver** for AArch64 and 
**getAvailableFeatures** for X86.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> nemanjai wrote:
> > arsenm wrote:
> > > nemanjai wrote:
> > > > arsenm wrote:
> > > > > From this description I don't understand what this is supposed to do. 
> > > > > What does the input mean? Why does this use an i32 immarg and not a 
> > > > > pointer? Why is the result only i32?
> > > > That is fair enough. The description is fairly vague. I can try to 
> > > > improve it as per below. The parameter for this is not a pointer (i.e. 
> > > > not an address). It is an immediate that represents the index into the 
> > > > enumeration of values that are provided at a fixed address. The back 
> > > > end is then free to produce the actual fixed address and the load 
> > > > itself.
> > > > The choice for the result type was admittedly arbitrary - on PPC, the 
> > > > values provided by GLIBC are 32-bit words.
> > > > 
> > > > Proposed comment describing this intrinsic:
> > > > ```
> > > > // This intrinsic is provided to allow back ends to emit load
> > > > // instructions that load a value from a fixed address. The
> > > > // parameter to the intrinsic is not an address, but an
> > > > // immediate index into an enumeration that contains the
> > > > // union of all such values available on all back ends.
> > > > // An example is the HWCAP/HWCAP2/CPUID words
> > > > // provided by GLIBC on PowerPC to allow fast access
> > > > // to commonly used parts of AUXV. These are provided
> > > > // at a fixed offset into the TCB (accessible through the
> > > > // thread pointer).
> > > > ```
> > > This is baking an a very target specific implementation of device 
> > > identification. Could you redefine this as something more abstract? Like 
> > > returns a device ID integer, or a bool that some int input is supported?
> > The idea is for this to not be restricted to device ID at all, but to be 
> > used for any values that reside in a fixed address for the compiler. For 
> > example, `STACK_GUARD` can be one of the values. How about if the intrinsic 
> > returns any integer type (or maybe any type)?
> > My thinking is that there may be need in the future for various things to 
> > be loaded from fixed addresses. The list of possible things that can be 
> > loaded this way would be a union of what all targets want and only specific 
> > values would make sense on each target.
> But you're assuming there's a fixed address this can be loaded from, and not 
> a read from a special register or some other mechanism
Oh, well sure. My intent was just for those things that are in memory at a 
fixed address. But I suppose you're right, there is probably not a fundamental 
reason to restrict it to things at fixed addresses.

What I am trying to avoid here is defining an intrinsic that takes a specific 
value and returns a `bool` (i.e. the intrinsic version of 
`__builtin_cpu_{supports|is}`. The features/cpus will be different for every 
target and mapping features to integers is also target specific. So this was an 
attempt to implement an intrinsic that gets a value (bit vector if you will) 
that the target can choose how to lower and what to do with the value (i.e. how 
to mask it).

I suppose each target can define their own intrinsics for accessing CPU 
identification information since each target has to provide code generation for 
them anyway.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6787
   }
+  case Intrinsic::fixed_addr_ld: {
+SDValue Chain = getRoot();

This should check `useLoadFixedAddr()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv added a comment.

In D152914#4497599 , @nemanjai wrote:

> I took a quick look at your patch. I think it would be preferable to make the 
> builtins target-independent rather than implementing the builtin by the same 
> name for multiple targets. Although I think it is very useful to support a 
> plus-separated list for `__builtin_cpu_supports()`, I think that's probably 
> something for a subsequent patch. We would need to figure out code generation 
> for that - perhaps that part will have to be completely target specific.

I fully agree with you to make  `__builtin_cpu_supports()` target-independent ( 
and I will update my patch on top of yours ). If plus-separated format is not 
supported by all target then I would suggest to make SemaBuiltinCpuSupports 
target-dependent - it will allow me to imlement plus-separated format on 
AArch64 keeping  `__builtin_cpu_supports()` target-independent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

nemanjai wrote:
> arsenm wrote:
> > nemanjai wrote:
> > > arsenm wrote:
> > > > From this description I don't understand what this is supposed to do. 
> > > > What does the input mean? Why does this use an i32 immarg and not a 
> > > > pointer? Why is the result only i32?
> > > That is fair enough. The description is fairly vague. I can try to 
> > > improve it as per below. The parameter for this is not a pointer (i.e. 
> > > not an address). It is an immediate that represents the index into the 
> > > enumeration of values that are provided at a fixed address. The back end 
> > > is then free to produce the actual fixed address and the load itself.
> > > The choice for the result type was admittedly arbitrary - on PPC, the 
> > > values provided by GLIBC are 32-bit words.
> > > 
> > > Proposed comment describing this intrinsic:
> > > ```
> > > // This intrinsic is provided to allow back ends to emit load
> > > // instructions that load a value from a fixed address. The
> > > // parameter to the intrinsic is not an address, but an
> > > // immediate index into an enumeration that contains the
> > > // union of all such values available on all back ends.
> > > // An example is the HWCAP/HWCAP2/CPUID words
> > > // provided by GLIBC on PowerPC to allow fast access
> > > // to commonly used parts of AUXV. These are provided
> > > // at a fixed offset into the TCB (accessible through the
> > > // thread pointer).
> > > ```
> > This is baking an a very target specific implementation of device 
> > identification. Could you redefine this as something more abstract? Like 
> > returns a device ID integer, or a bool that some int input is supported?
> The idea is for this to not be restricted to device ID at all, but to be used 
> for any values that reside in a fixed address for the compiler. For example, 
> `STACK_GUARD` can be one of the values. How about if the intrinsic returns 
> any integer type (or maybe any type)?
> My thinking is that there may be need in the future for various things to be 
> loaded from fixed addresses. The list of possible things that can be loaded 
> this way would be a union of what all targets want and only specific values 
> would make sense on each target.
But you're assuming there's a fixed address this can be loaded from, and not a 
read from a special register or some other mechanism


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> nemanjai wrote:
> > arsenm wrote:
> > > From this description I don't understand what this is supposed to do. 
> > > What does the input mean? Why does this use an i32 immarg and not a 
> > > pointer? Why is the result only i32?
> > That is fair enough. The description is fairly vague. I can try to improve 
> > it as per below. The parameter for this is not a pointer (i.e. not an 
> > address). It is an immediate that represents the index into the enumeration 
> > of values that are provided at a fixed address. The back end is then free 
> > to produce the actual fixed address and the load itself.
> > The choice for the result type was admittedly arbitrary - on PPC, the 
> > values provided by GLIBC are 32-bit words.
> > 
> > Proposed comment describing this intrinsic:
> > ```
> > // This intrinsic is provided to allow back ends to emit load
> > // instructions that load a value from a fixed address. The
> > // parameter to the intrinsic is not an address, but an
> > // immediate index into an enumeration that contains the
> > // union of all such values available on all back ends.
> > // An example is the HWCAP/HWCAP2/CPUID words
> > // provided by GLIBC on PowerPC to allow fast access
> > // to commonly used parts of AUXV. These are provided
> > // at a fixed offset into the TCB (accessible through the
> > // thread pointer).
> > ```
> This is baking an a very target specific implementation of device 
> identification. Could you redefine this as something more abstract? Like 
> returns a device ID integer, or a bool that some int input is supported?
The idea is for this to not be restricted to device ID at all, but to be used 
for any values that reside in a fixed address for the compiler. For example, 
`STACK_GUARD` can be one of the values. How about if the intrinsic returns any 
integer type (or maybe any type)?
My thinking is that there may be need in the future for various things to be 
loaded from fixed addresses. The list of possible things that can be loaded 
this way would be a union of what all targets want and only specific values 
would make sense on each target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

nemanjai wrote:
> arsenm wrote:
> > From this description I don't understand what this is supposed to do. What 
> > does the input mean? Why does this use an i32 immarg and not a pointer? Why 
> > is the result only i32?
> That is fair enough. The description is fairly vague. I can try to improve it 
> as per below. The parameter for this is not a pointer (i.e. not an address). 
> It is an immediate that represents the index into the enumeration of values 
> that are provided at a fixed address. The back end is then free to produce 
> the actual fixed address and the load itself.
> The choice for the result type was admittedly arbitrary - on PPC, the values 
> provided by GLIBC are 32-bit words.
> 
> Proposed comment describing this intrinsic:
> ```
> // This intrinsic is provided to allow back ends to emit load
> // instructions that load a value from a fixed address. The
> // parameter to the intrinsic is not an address, but an
> // immediate index into an enumeration that contains the
> // union of all such values available on all back ends.
> // An example is the HWCAP/HWCAP2/CPUID words
> // provided by GLIBC on PowerPC to allow fast access
> // to commonly used parts of AUXV. These are provided
> // at a fixed offset into the TCB (accessible through the
> // thread pointer).
> ```
This is baking an a very target specific implementation of device 
identification. Could you redefine this as something more abstract? Like 
returns a device ID integer, or a bool that some int input is supported?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

@arsenm Has my description adequately addressed your question? Do you think we 
should move forward with [some version of] this patch or do you have any 
fundamental objections?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D152914#4428692 , @ilinpv wrote:

> Thank you for the patch, it comes in the right time - we are also working on 
> AArch64 __builtin_cpu_supports, and I was thinking how to make it more 
> general.
> I uploaded our RFC version for review https://reviews.llvm.org/D153153
>
>
> It would be great to have in __builtin_cpu_supports argument string of 
> plus-separated features. Just SemaBuiltinCpuSupports need to handle this.

Hi Pavel,
I took a quick look at your patch. I think it would be preferable to make the 
builtins target-independent rather than implementing the builtin by the same 
name for multiple targets. Although I think it is very useful to support a 
plus-separated list for `__builtin_cpu_supports()`, I think that's probably 
something for a subsequent patch. We would need to figure out code generation 
for that - perhaps that part will have to be completely target specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Pavel Iliin via Phabricator via cfe-commits]

ilinpv added a comment.

Thank you for the patch, it comes in the right time - we are also working on 
AArch64 __builtin_cpu_supports, and I was thinking how to make it more general.
I uploaded our RFC version for review https://reviews.llvm.org/D153153
It would be great to have in __builtin_cpu_supports argument string of 
plus-separated features. Just SemaBuiltinCpuSupports need to handle this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

arsenm wrote:
> From this description I don't understand what this is supposed to do. What 
> does the input mean? Why does this use an i32 immarg and not a pointer? Why 
> is the result only i32?
That is fair enough. The description is fairly vague. I can try to improve it 
as per below. The parameter for this is not a pointer (i.e. not an address). It 
is an immediate that represents the index into the enumeration of values that 
are provided at a fixed address. The back end is then free to produce the 
actual fixed address and the load itself.
The choice for the result type was admittedly arbitrary - on PPC, the values 
provided by GLIBC are 32-bit words.

Proposed comment describing this intrinsic:
```
// This intrinsic is provided to allow back ends to emit load
// instructions that load a value from a fixed address. The
// parameter to the intrinsic is not an address, but an
// immediate index into an enumeration that contains the
// union of all such values available on all back ends.
// An example is the HWCAP/HWCAP2/CPUID words
// provided by GLIBC on PowerPC to allow fast access
// to commonly used parts of AUXV. These are provided
// at a fixed offset into the TCB (accessible through the
// thread pointer).
```



Comment at: llvm/include/llvm/IR/Intrinsics.td:905
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],

arsenm wrote:
> I think a new intrinsic should be in a separate commit 
Sounds good.



Comment at: llvm/include/llvm/Support/TargetOpcodes.def:148
 
+/// This pseudo-instruction loads a value provided by libc at afixed address.
+/// For example, GLIBC provides HWCAP, HWCAP2 and CPUID at a fixed address

arsenm wrote:
> Typo afixed
ACK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:903-907
+// Load of a value provided by the system library at a fixed address. Used for
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],
+[IntrInaccessibleMemOnly, ImmArg>]>;

From this description I don't understand what this is supposed to do. What does 
the input mean? Why does this use an i32 immarg and not a pointer? Why is the 
result only i32?



Comment at: llvm/include/llvm/IR/Intrinsics.td:905
+// accessing things like HWCAP word provided by GLIBC.
+def int_fixed_addr_ld
+: DefaultAttrsIntrinsic<[llvm_i32_ty], [llvm_i32_ty],

I think a new intrinsic should be in a separate commit 



Comment at: llvm/include/llvm/Support/TargetOpcodes.def:148
 
+/// This pseudo-instruction loads a value provided by libc at afixed address.
+/// For example, GLIBC provides HWCAP, HWCAP2 and CPUID at a fixed address

Typo afixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1785
 
+  if (static_cast(TM).hasGlibcHWCAPAccess())
+OutStreamer->emitSymbolValue(

This probably deserves a comment along the lines of:
```
// Emit a reference to a symbol exported by versions of GLIBC
// that provide the HWCAP access. If a program built with this
// is run on a system with an old GLIBC, it should cause a
// load-time error rather than loading garbage.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152914

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

[PATCH] D152914: [Draft] Make __builtin_cpu builtins target-independent

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: PowerPC, RKSimon, pengfei, arsenm, t.p.northover.
Herald added subscribers: steven.zhang, kbarton, hiraditya.
Herald added a project: All.
nemanjai requested review of this revision.
Herald added subscribers: jdoerfert, wdng.
Herald added projects: clang, LLVM.

This is just a proposal patch for a possible direction we can extend these 
builtins to other targets as well as an implementation for PowerPC.

This adds a new instruction similar to `LOAD_STACK_GUARD` that is meant to load 
things from fixed addresses. The new instruction has semantics that are a 
superset of what `LOAD_STACK_GUARD` does so the two can be folded into one.

My hope with this patch is to collect feedback from the community about the 
direction, so please provide any feedback you may have.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152914

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/BuiltinsX86.def
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-cpu-supports.c
  clang/test/Sema/builtin-cpu-supports.c
  llvm/include/llvm/Analysis/TargetLibraryInfo.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/include/llvm/Support/TargetOpcodes.def
  llvm/include/llvm/Target/Target.td
  llvm/include/llvm/TargetParser/PPCTargetParser.def
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
  llvm/lib/Target/PowerPC/PPCISelLowering.cpp
  llvm/lib/Target/PowerPC/PPCISelLowering.h
  llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
  llvm/lib/Target/PowerPC/PPCSubtarget.h
  llvm/lib/Target/PowerPC/PPCTargetMachine.h
  llvm/test/CodeGen/PowerPC/cpu-supports.ll

Index: llvm/test/CodeGen/PowerPC/cpu-supports.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/cpu-supports.ll
@@ -0,0 +1,111 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc64-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=BE64
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=BE32
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names \
+; RUN:   -mtriple=powerpc64le-linux-gnu < %s | FileCheck  %s \
+; RUN:   -check-prefix=LE
+define dso_local signext i32 @test(i32 noundef signext %a) local_unnamed_addr #0 {
+; BE64-LABEL: test:
+; BE64:   # %bb.0: # %entry
+; BE64-NEXT:lwz r4, -28772(r13)
+; BE64-NEXT:andis. r4, r4, 128
+; BE64-NEXT:bne cr0, .LBB0_3
+; BE64-NEXT:  # %bb.1: # %if.else
+; BE64-NEXT:lwz r4, -28776(r13)
+; BE64-NEXT:andis. r4, r4, 1024
+; BE64-NEXT:bne cr0, .LBB0_4
+; BE64-NEXT:  # %bb.2: # %if.else3
+; BE64-NEXT:lwz r4, -28764(r13)
+; BE64-NEXT:cmplwi r4, 39
+; BE64-NEXT:addi r4, r3, 5
+; BE64-NEXT:slwi r3, r3, 1
+; BE64-NEXT:iseleq r3, r3, r4
+; BE64-NEXT:  .LBB0_3: # %return
+; BE64-NEXT:extsw r3, r3
+; BE64-NEXT:blr
+; BE64-NEXT:  .LBB0_4: # %if.then2
+; BE64-NEXT:addi r3, r3, -5
+; BE64-NEXT:extsw r3, r3
+; BE64-NEXT:blr
+;
+; BE32-LABEL: test:
+; BE32:   # %bb.0: # %entry
+; BE32-NEXT:lwz r4, -28732(r2)
+; BE32-NEXT:andis. r4, r4, 128
+; BE32-NEXT:bnelr cr0
+; BE32-NEXT:  # %bb.1: # %if.else
+; BE32-NEXT:lwz r4, -28736(r2)
+; BE32-NEXT:andis. r4, r4, 1024
+; BE32-NEXT:bne cr0, .LBB0_3
+; BE32-NEXT:  # %bb.2: # %if.else3
+; BE32-NEXT:lwz r4, -28724(r2)
+; BE32-NEXT:cmplwi r4, 39
+; BE32-NEXT:addi r4, r3, 5
+; BE32-NEXT:slwi r3, r3, 1
+; BE32-NEXT:iseleq r3, r3, r4
+; BE32-NEXT:blr
+; BE32-NEXT:  .LBB0_3: # %if.then2
+; BE32-NEXT:addi r3, r3, -5
+; BE32-NEXT:blr
+;
+; LE-LABEL: test:
+; LE:   # %bb.0: # %entry
+; LE-NEXT:lwz r4, -28776(r13)
+; LE-NEXT:andis. r4, r4, 128
+; LE-NEXT:bne cr0, .LBB0_3
+; LE-NEXT:  # %bb.1: # %if.else
+; LE-NEXT:lwz r4, -28772(r13)
+; LE-NEXT:andis. r4, r4, 1024
+; LE-NEXT:bne cr0, .LBB0_4
+; LE-NEXT:  # %bb.2: # %if.else3
+; LE-NEXT:lwz r4, -28764(r13)
+; LE-NEXT:cmplwi r4, 39
+; LE-NEXT:addi r4, r3, 5
+; LE-NEXT:slwi r3, r3, 1
+; LE-NEXT:iseleq r3, r3, r4
+; LE-NEXT:  .LBB0_3: # %return
+; LE-NEXT:extsw r3, r3
+; LE-NEXT:blr
+; LE-NEXT:  .LBB0_4: # %if.then2
+; LE-NEXT:addi r3, r3, -5
+; LE-NEXT:extsw r3, r3
+; LE-NEXT:blr
+entry:
+  %cpu_supports = tail call i32 @llvm.fixed.addr.ld(i32 1)
+  %0 = and i32 %cpu_supports, 8388608
+  %.not = icmp eq i32 %0, 0
+  br i1 %.not, label %if.else, label %return
+
+if.else:  ; preds = %entry
+  %cpu_supports1 = tail call