[llvm-bugs] [Bug 31774] New: LLVM generates poor code for MSVC std::string::~string due to memalign-like code in std::allocator<T>::deallocate

[via llvm-bugs]

https://llvm.org/bugs/show_bug.cgi?id=31774

            Bug ID: 31774
           Summary: LLVM generates poor code for MSVC std::string::~string
                    due to memalign-like code in
                    std::allocator<T>::deallocate
           Product: libraries
           Version: trunk
          Hardware: PC
                OS: Windows NT
            Status: NEW
          Severity: normal
          Priority: P
         Component: Transformation Utilities
          Assignee: unassignedbugs at nondot.org
          Reporter: rnk at google.com
                CC: llvm-bugs at lists.llvm.org
    Classification: Unclassified

There are many temporary std::strings in Chrome, and they are destroyed
frequently. LLVM inlines std::string and the things it calls, like
std::allocator<char>::deallocate. That calls std::_Deallocate in <xmemory0>,
which has this memalign-like code in it:
https://github.com/icestudent/vc-19-changes/blob/master/xmemory0#L97

This is what it looks like after pre-processing and reformatting:
inline void _Deallocate(void* _Ptr, size_t _Count, size_t _Sz) {
  const size_t _User_size = _Count * _Sz;
  if (4096 <= _User_size) {
    const uintptr_t _Ptr_user = reinterpret_cast<uintptr_t>(_Ptr);
    {
      if (!((_Ptr_user & (32 - 1)) == 0)) {
        ::_invalid_parameter_noinfo_noreturn();
      }
    };
    const uintptr_t _Ptr_ptr = _Ptr_user - sizeof(void*);
    const uintptr_t _Ptr_container = *reinterpret_cast<uintptr_t*>(_Ptr_ptr);
    {
      if (!(_Ptr_container < _Ptr_user)) {
        ::_invalid_parameter_noinfo_noreturn();
      }
      if (!(sizeof(void*) <= _Ptr_user - _Ptr_container)) {
        ::_invalid_parameter_noinfo_noreturn();
      }
      if (!(_Ptr_user - _Ptr_container <= (sizeof(void*) + 32 - 1))) {
        ::_invalid_parameter_noinfo_noreturn();
      }
    };
    _Ptr = reinterpret_cast<void*>(_Ptr_container);
  }
  ::operator delete(_Ptr);
}

This causes two problems:
1. excessive code size because most strings are < 4096 bytes
2. blocks new / delete pair removal by obscuring the _Ptr argument to delete

The code size issue would be addressed by commoning the calls to
::_invalid_parameter_noinfo_noreturn(), which surprisingly we don't do.

One way to look at it is that we should canonicalize f and g to the same code:

void __attribute__((noreturn)) abort();
void f(unsigned x, unsigned y) {
  if (x >= y)
    abort();
  int i = y - x;
  if (i < 7)
    abort();
  if (i > 40)
    abort();
}
void g(unsigned x, unsigned y) {
  if (!(x < y) || (y - x) < 7 || (y - x) > 40)
    abort();
}

Fixing 1 seems like a nice general improvement. I know in the past tail merging
was considered to be non-canonical, but tail merging the abort calls here
unlocks a lot of instcombine goodness, so it might be worth revisiting that.

To fix 2, we should probably recognize std::_Deallocate and std::_Allocate
pairs as allocation functions like new and delete, and avoid inlining them in
the first place.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20170126/6abd3017/attachment-0001.html>