Bump on this when reviewers have a chance. Thanks!

Bump on this when reviewers have a chance. Thanks!

Bump on this when reviewers have a chance. Thanks!

Abandoning in favor of https://github.com/llvm/llvm-project/pull/65245.

Address reviewer feedback

• Switch main test to use regular expressions and lit variables for

verification that hashes are correct.

Skip printing hashes of function declarations.

Fixes https://github.com/llvm/llvm-project/issues/64938.

Bump on this patch when reviewers have a chance. Thanks!

Address reviewer feedback

• Add handling for vector return types

Switch to modifying ForceFunctionAttrs rather than adding an opt option.

Once you adopt @MaskRay's comments regarding the comment wording and preprocessor directives, this should LGTM.

Any opposition to landing this with the detailed parameter set to false in the PM changed checks temporarily while https://github.com/llvm/llvm-project/issues/64938 is fixed (and then enable detailed structural hashing for change status expensive checks once that is fixed)?

@jdoerfert also suggested doing this in an IPO pass (like FunctionAttrs) but it seemed simple enough to implement this way. I couldn't think of any benefit of doing it within an IPO pass, but definitely could be missing something. Also wondering if there are reasons for not doing this as I thought there would've been something like this already.

Address reviewer feedback

Address reviewer feedback

• Fix issue with critical edge counting

Looks like the changes caught a regression:

-- Testing: 1 tests, 1 workers --
FAIL: LLVM :: CodeGen/X86/statepoint-stack-usage.ll (1 of 1)
******************** TEST 'LLVM :: CodeGen/X86/statepoint-stack-usage.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /tmp/llvm-expensive-checks/bin/llc -verify-machineinstrs -stack-symbol-ordering=0 < /tmp/llvm-project/llvm/test/CodeGen/X86/statepoint-stack-usage.ll | /tmp/llvm-expensive-checks/bin/FileCheck /tmp/llvm-project/llvm/test/CodeGen/X86/statepoint-stack-usage.ll
--
Exit Code: 2
Command Output (stderr):
--
+ : 'RUN: at line 1'
+ /tmp/llvm-expensive-checks/bin/llc -verify-machineinstrs -stack-symbol-ordering=0
+ /tmp/llvm-expensive-checks/bin/FileCheck /tmp/llvm-project/llvm/test/CodeGen/X86/statepoint-stack-usage.ll
Pass modifies its input and doesn't report it: CodeGen Prepare
Pass modifies its input and doesn't report it
UNREACHABLE executed at /tmp/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1441!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /tmp/llvm-expensive-checks/bin/llc -verify-machineinstrs -stack-symbol-ordering=0
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'CodeGen Prepare' on function '@back_to_back_calls'
 #0 0x00005555580db4c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/llvm-expensive-checks/bin/llc+0x2b874c8)
 #1 0x00005555580d8f7e llvm::sys::RunSignalHandlers() (/tmp/llvm-expensive-checks/bin/llc+0x2b84f7e)
 #2 0x00005555580dbca8 SignalHandler(int) Signals.cpp:0:0
 #3 0x0000155554fd9520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x000015555502da7c pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x96a7c)
 #5 0x0000155554fd9476 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42476)
 #6 0x0000155554fbf7f3 abort (/lib/x86_64-linux-gnu/libc.so.6+0x287f3)
 #7 0x0000555558038700 llvm::install_out_of_memory_new_handler() (/tmp/llvm-expensive-checks/bin/llc+0x2ae4700)
 #8 0x0000555557922b42 (/tmp/llvm-expensive-checks/bin/llc+0x23ceb42)
 #9 0x000055555792b232 llvm::FPPassManager::runOnModule(llvm::Module&) (/tmp/llvm-expensive-checks/bin/llc+0x23d7232)
#10 0x00005555579233fa llvm::legacy::PassManagerImpl::run(llvm::Module&) (/tmp/llvm-expensive-checks/bin/llc+0x23cf3fa)
#11 0x0000555556a5e757 main (/tmp/llvm-expensive-checks/bin/llc+0x150a757)
#12 0x0000155554fc0d90 (/lib/x86_64-linux-gnu/libc.so.6+0x29d90)
#13 0x0000155554fc0e40 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e40)
#14 0x0000555556a58c65 _start (/tmp/llvm-expensive-checks/bin/llc+0x1504c65)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /tmp/llvm-expensive-checks/bin/FileCheck /tmp/llvm-project/llvm/test/CodeGen/X86/statepoint-stack-usage.ll
--
********************
********************
Failed Tests (1):
  LLVM :: CodeGen/X86/statepoint-stack-usage.ll

Address reviewer feedback

• Remove TODO comment

In D158317#4611235, @aidengrossman wrote:

Address reviewer feedback

• Switch pass option name to detailed rather than the much more verbose enable-detailed-structural-hash

Address reviewer feedback

• Switch pass option name to detailed rather than the much more verbose enable-detailed-structural-hash

In D158317#4611131, @nikic wrote:

It would probably make sense to move your existing unit test coverage over to the printer pass.

Update test that I forgot to save before committing.

Split lit test

Address reviewer feedback

• Update test hashes based on most recent revision of earlier patch in stack
• Make test require 64 bit architecture to prevent failures on 32 bit arches.

Address reviewer feedback

Update patch

• Switch to native hashing implementations for APInt/APFloat
• Switch from hashing type pointers to hashing some details about the type as the former is non-deterministic (which of course is frustratingly masked by my system having ASLR disabled). Left a TODO to refactor the type hashing into a more formal hash_value implementation within the class.
• Formatting changes.

Address reviewer feedback

• Fix top comment typo
• Remove mostly useless header comment
• Switch to using pass options fron the cl::opt in printer pass
• Update test

Fix minor formatting issue

Address reviewer feedback

• Early exit where pointed out.
• Switch to hashing operands for the most part (except for in the case of cmp)
• Add a couple additional operands to hash, particularly integer constants and arguments.
• Rebase against upstream to include changes to prevent test regressions on 32-bit arches.

It looks like hash_code stores everything as size_t which causes the difference between the 32 bit and 64 bit platforms. The functions end up in different places as MergeFunctions ends up ordering them by hash. I've switched the hashing a little bit (mainly using uint64_t and switching to hash_16_bytes from hash_combine to get rid of the size_t) so that things are consistent across platforms. I've also updated the magic constant as the function header to prevent any unnecessary churn in the test diffs and I've verified that behavior is consistent between 64 and 32 bit builds now.

This is an updated version of https://reviews.llvm.org/D150646 that uses __has_builtin instead of checking for a definition of _MSC_EXTENSIONS. This fixes the cases that I have received so far and shouldn't cause any other issues.

Thanks for bringing this to my attention! Not sure how I completely missed the buildbot notifications. I've reverted for now so that I can work on a fix out-of-trunk and will resubmit once I have this fixed (and after more thorough testing).

I believe that /Ze has more in common with -fms-compatibility than -fms-extensions, but I could be wrong. Also, they may just be completely different things at this point. /permissive is closer in spirit to -fms-compatibility.

Address reviewer feedback

• Add comment that different users might want different behavior so options

should be preferred.

In D157334#4596328, @rnk wrote:

I don't think this is the right thing to do, I think I recall saying as much on some other review. The MSVC docs say that /Za//Ze control _MSC_EXTENSION, and as I understand it, /Za is more like our -fms-compatibility flag, so it makes sense to control this macro with -fms-compatibility.