[PATCH] D69998: [MacroFusion] Create the missing artificial edges if there are more than 2 SU fused.

Thu Dec 5 13:31:36 PST 2019

fhahn added a comment.

In D69998#1769958 <https://reviews.llvm.org/D69998#1769958>, @steven.zhang wrote:

> In D69998#1768588 <https://reviews.llvm.org/D69998#1768588>, @fhahn wrote:
>
> > With rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba <https://reviews.llvm.org/rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba> landed, is this still relevant?
>
>
> Yes, they are different problems. rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba <https://reviews.llvm.org/rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba> is trying to limit the number of chained SU's as two, while this patch is to fix the problem if we want to chain more than two SU's, though it is limited to two now. But by the design, we should have it work well if someone want to relax the limit later. It is somewhat like, we have the ability to chain any number of SU's, but now, it is limited to two, instead of, we can only chain two SU's, and have bugs if chain more.

Sure, but currently there can be no bug and the interface prevents that case from happening. I don't see why we would need to deal with a case that might happen in the future, if the interface changes. To me it seems like the time to fix that would be when the interface gets extended. Until then, we cannot test the patch. To support fusing more than pairs, I think it would be better to do this in a separate function and deal with those cases there, rather than unnecessarily complicating the code for pairs.

> There won't be any compiling time impact for this patch if it is limited to two, as the pred/succ of CurrentSU is always null if only chain two SU's.

Hm I do not think that's true, we still need to check all the predecessors/successors of the SUs, which potentially can be a large number for bad inputs. The compile-time impact of this patch on its own might be quite small, but at least in degenerate cases it could be measurable (same for rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba <https://reviews.llvm.org/rGd84b320dfd0a7dbedacc287ede5e5bc4c0f113ba> )

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

https://reviews.llvm.org/D69998