rnk added a comment.

This change needs a clang CodeGen test to show that we generate IR with 
`__ptr32` / `__ptr64` in the correct places. We should already have some 
semantic tests, but we may need more.



================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1874
+    case LangAS::ptr32_sptr:
+      Extra.mangleSourceName("_ASPtr32_sptr");
+      break;
----------------
Hm, we should actually mangle these as they do. See the FIXME comment above the 
definition of `PointersAre64Bit`. If you look at the uses of PointersAre64Bit, 
you should be able to find the places where you need to check if a pointer type 
is either in the explicit 64-bit address space or in the default address space 
for a 64-bit target. That check sounds like a good helper function.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:2874
                              N->getUnqualifiedType())) {
+      const PointerType *OldTypePtr =
+          dyn_cast<PointerType>(O->getUnqualifiedType());
----------------
This probably deserves a comment. It looks like this lets you do this:
```
void foo(int *__ptr64);
void foo(int *p) { } // assume x64 target
```
... without having the compiler think it's creating an overload.

Separately, MSVC doesn't permit `__ptr32/__ptr64` overloads. Is it possible to 
implement that here as well?


================
Comment at: llvm/test/CodeGen/X86/mixed-ptr-sizes.ll:11
+;   f->p64 = i;
+;   use_foo(f);
+; }
----------------
Do you need use_foo? I think `f` is a parameter, so the compiler can't remove 
any stores to it, even without a call to use it. You should be able to simplify 
the test to skip these calls.


================
Comment at: llvm/test/CodeGen/X86/mixed-ptr-sizes.ll:30
+;
+; $ clang -cc1 -triple x86_64-windows-msvc -fms-extensions -O2 -S t.cpp
+
----------------
If you compile this code as plain C code, then it won't have as much name 
mangling, which makes the .ll file more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66827



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

Reply via email to