[Sim] Flatten format string concatenations in canonicalizer #7316
Conversation
| if (!op.isFlat()) { | ||
| // Get a new, flattened list of operands | ||
| flatOperands.reserve(op.getNumOperands() + 4); | ||
| auto isAcyclic = op.getFlattenedInputs(flatOperands); | ||
|
|
||
| if (failed(isAcyclic)) { | ||
| // Infinite recursion, but we cannot fail compilation right here (can we?) | ||
| // so just emit a warning and bail out. | ||
| op.emitWarning("Cyclic concatenation detected."); | ||
| return failure(); | ||
| } | ||
|
|
||
| hasBeenFlattened = true; | ||
| } |
There was a problem hiding this comment.
I'm wondering if we even have to emit diagnostics and handled failure cases here. I think the canonicalization is intended to be purely opportunistic, so if we encounter a cycle we should be able to just silently not canonicalize and return failure. Or in case getFlattenedInputs creates a valid list of operands even if a cycle is present, we could just work with that and ignore the cycle altogether. Especially since the verifier catches this already 😃
There was a problem hiding this comment.
That is actually how I had done this originally. I then threw it out for a potentially silly reason: If we flatten despite the presence of cycles, it becomes difficult to test as it is hard to predict (and potentially unstable for future LLVM bumps?) which op is going to fail the verification first. And annoyingly I could not find any way to tell the -verify-diagnostics framework that the error may be produced at different locations. Quite possibly I'm overthinking this (I usually do). But in the end I decided to just keep it as simple as possible, given that:
- A cyclic concatenation would almost certainly be indicative of a bug in the frontend.
- It is pretty much guaranteed to cause a failure in the subsequent lowering pass.
I kept the warning in case something goes wrong and the cycle causes an infinite loop, so it doesn't just freeze quietly.
There was a problem hiding this comment.
That makes sense! Very cool stuff in any case 😎 🥳!
Provide an interface to get the flat format string for
sim.fmt.concatoperations and opportunistically flatten during canonicalization.CC #7292