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

Fri Nov 18 08:56:58 PST 2022

spatel added a comment.

In D137791#3935304 <https://reviews.llvm.org/D137791#3935304>, @jonpa wrote:

> 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?

It's a trade-off in test fragility / specificity. As noted, we're not using any scheduler options on the RUN line now (so the test is more focused, but the actual miscompile is not visible). 
I can add another RUN to check the debug spew from DAGCombiner, but I don't see how that makes the test intent any clearer than the MIR checks + test comment.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8115
     Stores.push_back(Store);
+
+    // If this store is used as a chain operand in a store that is not included
----------------
jonpa wrote:
> I think this will also bail on stores that are actually not aliasing any of the stores, for instance the store to '@d' in the test case. Would it be possible to check aliasing against the full, merged store? (maybe in a separate loop further down after the offsets have been checked?)
> 
> Why are not the uses of N checked?
> 
> What about the uses of the uses... wouldn't we have to check any additional stores further down the DAG via the use edges?
> 
> Does this make a big difference? I am thinking that an alternative to bailing might be to add the correct chains from the new store to all the users..?
> 
> 
TBH, I'm not interested in preserving corner-case optimizations that are not based on the motivating code patterns (for which there are several regression tests across different targets).

I just want to avoid the proven miscompile; that's the most important thing. Optimizations can be added later if that matters.

I don't think it is necessary to walk uses of uses on this transform. IIUC, if there's some other problematic store that is included anywhere on this chain, then it must be a use of at least one of the stores to be merged, so we should find it eventually (and give up on the transform). 

I don't have a complete understanding of chains and token factors, so I could be wrong, but I haven't come up with any code examples where this does not work.

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

https://reviews.llvm.org/D137791