[PATCH] D67350: [IfCvt][ARM] Optimise diamond if-conversion for code size

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard marked 4 inline comments as done.
ostannard added inline comments.


================
Comment at: llvm/lib/CodeGen/IfConversion.cpp:309
+        // When if-converting a diamond, the branches at the ends of all the
+        // true block, false block and common predecessor will all be removed.
+        for (auto &I : make_range(TIE, TBBInfo.BB->end())) {
----------------
efriedma wrote:
> Is this actually true, if the terminator isn't analyzable?  We can form a "diamond" where TrueBB and FalseBB have an arbitrary terminator, like an INLINEASM_BR, as long as it's the same.
Good point, non-analyzable terminators should be treated as common instructions, not branches which can be completely removed.


================
Comment at: llvm/lib/CodeGen/IfConversion.cpp:337
+        // their code size.
+        CommonBytes /= 2;
+
----------------
efriedma wrote:
> Are you adding CommonBytes for common instructions before the predicated ones, somewhere?
I didn't do that because I haven't been able to find an example where there are common instructions at the start of the blocks, I'd expect them to always be hoisted up into the common block. That said, the code change is small, and can be tested by MIR, so I'll add this.


================
Comment at: llvm/lib/CodeGen/IfConversion.cpp:985
+  TrueBBICalc.IsBrAnalyzable = TrueBBI.IsBrAnalyzable;
+  FalseBBICalc.IsBrAnalyzable = FalseBBI.IsBrAnalyzable;
   if (!RescanInstructions(TIB, FIB, TIE, FIE, TrueBBICalc, FalseBBICalc))
----------------
efriedma wrote:
> This looks weird; why are we constructing TrueBBICalc/FalseBBICalc like this?  What does this change do?
These are temporary `BBInfo` objects which will be passed into `MeetIfcvtSizeLimit`, I think they differ from the actual BBInfos in that they ignore the common instructions, which would prevent if-conversion if there wasn't a matching instruction in the other block.

I need to also pass through the `IsBrAnalyzable` value so that `MeetIfcvtSizeLimit` can treat non-analyzable branches differently to analyzable ones (only the latter can be removed completely).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67350