[PATCH] D58718: [Memory] Add basic support for large/huge memory pages

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Support/Windows/Memory.inc:59
 
+size_t getLargePageSize() {
+  HANDLE Token = 0;
----------------
Can we mark this function `static`?


================
Comment at: lib/Support/Windows/Memory.inc:65
+                     &Token);
+  if (Token) {
+    LUID Luid;
----------------
Can you do an early out here if it failed?


================
Comment at: lib/Support/Windows/Memory.inc:72
+      TP.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
+      if (AdjustTokenPrivileges(Token, FALSE, &TP, 0, 0, 0)) {
+        DWORD E = GetLastError();
----------------
Why do we actually even need to set this privilege on the process token?  Will huge pages not work without it?

Also, it's strange to be doing this from a function called `getLargePageSize` which sounds like it doesn't modify anything.


================
Comment at: lib/Support/Windows/Memory.inc:75
+        if (E == ERROR_SUCCESS) {
+          return LargePageMin;
+        }
----------------
There's a handle leak here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58718