[PATCH] D17092: [X86] Add -mseparate-stack-seg

[Michael LeMay via Phabricator via cfe-commits]

mlemay-intel updated this revision to Diff 87453.
mlemay-intel added a comment.

Removed the portions that are specific to 32-bit segmentation.  I plan to 
resubmit those later as a separate patch.


https://reviews.llvm.org/D17092

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1605,6 +1605,7 @@
 def mno_xsaves : Flag<["-"], "mno-xsaves">, Group;
 def mno_mwaitx : Flag<["-"], "mno-mwaitx">, Group;
 def mno_pku : Flag<["-"], "mno-pku">, Group;
+def mno_separate_stack_seg : Flag<["-"], "mno-separate-stack-seg">, 
Group;
 
 def munaligned_access : Flag<["-"], "munaligned-access">, 
Group,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;
@@ -1785,6 +1786,7 @@
 def mprfchw : Flag<["-"], "mprfchw">, Group;
 def mrdseed : Flag<["-"], "mrdseed">, Group;
 def mpku : Flag<["-"], "mpku">, Group;
+def mseparate_stack_seg : Flag<["-"], "mseparate-stack-seg">, 
Group;
 def madx : Flag<["-"], "madx">, Group;
 def msha : Flag<["-"], "msha">, Group;
 def mcx16 : Flag<["-"], "mcx16">, Group;


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1605,6 +1605,7 @@
 def mno_xsaves : Flag<["-"], "mno-xsaves">, Group;
 def mno_mwaitx : Flag<["-"], "mno-mwaitx">, Group;
 def mno_pku : Flag<["-"], "mno-pku">, Group;
+def mno_separate_stack_seg : Flag<["-"], "mno-separate-stack-seg">, Group;
 
 def munaligned_access : Flag<["-"], "munaligned-access">, Group,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;
@@ -1785,6 +1786,7 @@
 def mprfchw : Flag<["-"], "mprfchw">, Group;
 def mrdseed : Flag<["-"], "mrdseed">, Group;
 def mpku : Flag<["-"], "mpku">, Group;
+def mseparate_stack_seg : Flag<["-"], "mseparate-stack-seg">, Group;
 def madx : Flag<["-"], "madx">, Group;
 def msha : Flag<["-"], "msha">, Group;
 def mcx16 : Flag<["-"], "mcx16">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17092: [X86] Add -mseparate-stack-seg

[Michael Kuperstein via cfe-commits]

mkuper resigned from this revision.
mkuper removed a reviewer: mkuper.
mkuper added a comment.

I really don't know enough about this part of clang either.


http://reviews.llvm.org/D17092



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

Re: [PATCH] D17092: [X86] Add -mseparate-stack-seg

[Michael LeMay via cfe-commits]

mlemay-intel updated this revision to Diff 53920.
mlemay-intel added a comment.

Revise patch to fix line alignment.


http://reviews.llvm.org/D17092

Files:
  include/clang/Driver/Options.td
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/varargs.c

Index: test/CodeGen/varargs.c
===
--- test/CodeGen/varargs.c
+++ test/CodeGen/varargs.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck 
%s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -target-feature 
+separate-stack-seg -emit-llvm -o - %s | FileCheck -check-prefix=SEPARATE-SS %s
 
 // PR6433 - Don't crash on va_arg(typedef).
 typedef double gdouble;
@@ -20,4 +21,5 @@
   __builtin_va_list ap;
   void *p;
   p = __builtin_va_arg(ap, typeof (int (*)[++n])); // CHECK: add nsw i32 
{{.*}}, 1
+  // SEPARATE-SS: load i32*, i32* addrspace(258)* {{.*}}
 }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1690,9 +1690,26 @@
   TypeInfo.second = CharUnits::fromQuantity(
 getTypeStackAlignInBytes(Ty, TypeInfo.second.getQuantity()));
 
-  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false,
-  TypeInfo, CharUnits::fromQuantity(4),
-  /*AllowHigherAlign*/ true);
+  const Address Addr = emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ 
false,
+TypeInfo, CharUnits::fromQuantity(4),
+/*AllowHigherAlign*/ true);
+
+  const std::vector  =
+CGF.getTarget().getTargetOpts().Features;
+  if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+"+separate-stack-seg") != TargetFeatures.end()) {
+// Cast the pointer into the address space for the stack segment.
+// This is to help support multi-segment memory models in which DS and SS
+// may differ from each other.
+llvm::Type *DirectTy = CGF.ConvertTypeForMem(Ty);
+llvm::Value *PtrAsInt =
+  CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+llvm::Value *PtrInStackSeg =
+  CGF.Builder.CreateIntToPtr(PtrAsInt, DirectTy->getPointerTo(258));
+return Address(PtrInStackSeg, Addr.getAlignment());
+  }
+
+  return Addr;
 }
 
 bool X86_32TargetCodeGenInfo::isStructReturnInRegABI(
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1407,6 +1407,7 @@
 def mno_xsavec : Flag<["-"], "mno-xsavec">, Group;
 def mno_xsaves : Flag<["-"], "mno-xsaves">, Group;
 def mno_pku : Flag<["-"], "mno-pku">, Group;
+def mseparate_stack_seg : Flag<["-"], "mseparate-stack-seg">, 
Group;
 
 def munaligned_access : Flag<["-"], "munaligned-access">, 
Group,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;


Index: test/CodeGen/varargs.c
===
--- test/CodeGen/varargs.c
+++ test/CodeGen/varargs.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -target-feature +separate-stack-seg -emit-llvm -o - %s | FileCheck -check-prefix=SEPARATE-SS %s
 
 // PR6433 - Don't crash on va_arg(typedef).
 typedef double gdouble;
@@ -20,4 +21,5 @@
   __builtin_va_list ap;
   void *p;
   p = __builtin_va_arg(ap, typeof (int (*)[++n])); // CHECK: add nsw i32 {{.*}}, 1
+  // SEPARATE-SS: load i32*, i32* addrspace(258)* {{.*}}
 }
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1690,9 +1690,26 @@
   TypeInfo.second = CharUnits::fromQuantity(
 getTypeStackAlignInBytes(Ty, TypeInfo.second.getQuantity()));
 
-  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false,
-  TypeInfo, CharUnits::fromQuantity(4),
-  /*AllowHigherAlign*/ true);
+  const Address Addr = emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false,
+TypeInfo, CharUnits::fromQuantity(4),
+/*AllowHigherAlign*/ true);
+
+  const std::vector  =
+CGF.getTarget().getTargetOpts().Features;
+  if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+"+separate-stack-seg") != TargetFeatures.end()) {
+// Cast the pointer into the address space for the stack segment.
+// This is to help support multi-segment memory models in which DS and SS
+// may differ from each other.
+llvm::Type *DirectTy = CGF.ConvertTypeForMem(Ty);
+llvm::Value *PtrAsInt =
+  CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+llvm::Value *PtrInStackSeg =
+  

Re: [PATCH] D17092: [X86] Add -mseparate-stack-seg

[Michael LeMay via cfe-commits]

mlemay-intel added a comment.

Thank you for your feedback!



Comment at: lib/CodeGen/TargetInfo.cpp:1569
@@ +1568,3 @@
+CGF.getTarget().getTargetOpts().Features;
+  if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+"+separate-stack-seg") != TargetFeatures.end()) {

delena wrote:
> Hi,
> I'm not clang reviewer at all and you can ignore my comment.
> 
> Searching string looks strange for me. I suppose that this string should be 
> defined in another form. Something like
> options::OPT_separate_stack_seg.
> 
I haven't quite found an example of how an option like this should be handled.  
String comparisons of this form are performed in clang/lib/Basic/Targets.cpp, 
but there they are used to initialize variables.  Furthermore, all of those 
tests are for CPU features, whereas this one is for a particular segment 
configuration.


Comment at: lib/CodeGen/TargetInfo.cpp:1577
@@ +1576,3 @@
+  CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+   
DirectTy->getPointerTo(258));

delena wrote:
> You should not use 258 as a constant. It should be defined somewhere as 
> address space enum.
I agree with this principle.  However, the address space numbers for FS and GS 
are listed in the Clang documentation [1] and are used as literals in LLVM 
code.  Thus, I think this patch is consistent with existing usage of address 
space numbers.

[1] 
http://clang.llvm.org/docs/LanguageExtensions.html#memory-references-off-the-gs-segment


Comment at: lib/CodeGen/TargetInfo.cpp:1578
@@ +1577,3 @@
+llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+   
DirectTy->getPointerTo(258));
+return Address(PtrInStackSeg, Addr.getAlignment());

delena wrote:
> This line alignment does not match LLVM style.
I'll fix it.


Comment at: lib/CodeGen/TargetInfo.cpp:1580
@@ +1579,3 @@
+return Address(PtrInStackSeg, Addr.getAlignment());
+  }
+

delena wrote:
> Again, not sure that I'm right. You are trying to create addressspacecast. Is 
> it the right way to create ptrtoint + inttoptr?
I first attempted to create an address space cast, but LLVM disallows that.  I 
think it is necessary to perform the conversion through an int, but I am not 
entirely sure of that.


http://reviews.llvm.org/D17092



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

Re: [PATCH] D17092: [X86] Add -mseparate-stack-seg

[Elena Demikhovsky via cfe-commits]

delena added a subscriber: delena.


Comment at: lib/CodeGen/TargetInfo.cpp:1569
@@ +1568,3 @@
+CGF.getTarget().getTargetOpts().Features;
+  if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+"+separate-stack-seg") != TargetFeatures.end()) {

Hi,
I'm not clang reviewer at all and you can ignore my comment.

Searching string looks strange for me. I suppose that this string should be 
defined in another form. Something like
options::OPT_separate_stack_seg.



Comment at: lib/CodeGen/TargetInfo.cpp:1577
@@ +1576,3 @@
+  CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+   
DirectTy->getPointerTo(258));

You should not use 258 as a constant. It should be defined somewhere as address 
space enum.


Comment at: lib/CodeGen/TargetInfo.cpp:1578
@@ +1577,3 @@
+llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+   
DirectTy->getPointerTo(258));
+return Address(PtrInStackSeg, Addr.getAlignment());

This line alignment does not match LLVM style.


Comment at: lib/CodeGen/TargetInfo.cpp:1580
@@ +1579,3 @@
+return Address(PtrInStackSeg, Addr.getAlignment());
+  }
+

Again, not sure that I'm right. You are trying to create addressspacecast. Is 
it the right way to create ptrtoint + inttoptr?


http://reviews.llvm.org/D17092



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