[PATCH] D137791: [SDAG] bail out of mergeTruncStores() if there's an unknown store in the chain

Thu Nov 17 16:26:47 PST 2022

jonpa added a comment.

In D137791#3934204 <https://reviews.llvm.org/D137791#3934204>, @spatel wrote:

> In D137791#3934137 <https://reviews.llvm.org/D137791#3934137>, @jonpa wrote:
>
>> The test looks ok, but I guess if you wanted to you could instead have a test which contains "CHECK-NOT: STG", to make it explicit that there should be no 64-bit store in the output...
>
> I actually prefer to not use CHECK-NOT; we've seen cases where tests sporadically fail on those because someone's home dir or some other unexpected string pollutes the output and gets caught by FileCheck on just that one machine.
>
> The complete CHECK-NEXT sequence from start of the function through return guarantees that we have the expected code **and nothing else**. And it doesn't require any special target knowledge to adjust the test in the future if it changes since the CHECKs were auto-generated - someone with SystemZ knowledge would still review any diffs of course. :)

ok... a regexp like STG .* (store s64) might be fairly safe, but I guess the auto-generated test also may be simpler to maintain. However, I see that this test is actually now passing without the patch. I guess the extra options used earlier would make it fail, but maybe it would be best to really test the DAGCombiner:

-debug-only=dagcombine
REQUIRES: asserts
CHECK: Combining: t16: ch = store<(store (s32) into %ir.f4)> t13, t9, t15, undef:i64                  
CHECK-NOT ... into: t30: ch = store<(store (s64) into %ir.t1, align 4)> t12, t4, t11, undef:i64         
(better use some regexps)

That would also be more safe against any future changes to the scheduler options used originally, right?

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

https://reviews.llvm.org/D137791