[GlobalISel] Hide hoisted constants behind G_BITCAST to prevent folding.

[aemerson]

Summary

The constant hoisting pass tries to hoist large constants into predecessors and also
generates remat instructions in terms of the hoisted constants. These aim to prevent
codegen from rematerializing expensive constants multiple times. So we can re-use
this optimization, we can preserve the no-op bitcasts that are used to anchor
constants to the predecessor blocks.

SelectionDAG achieves this by having the OpaqueConstant node, which is just a
normal constant with an opaque flag set. I've opted to avoid introducing a new
constant generic instruction here.

However, once we've run all the combiners we can eliminate these G_BITCASTS
to allow selection to see them. For AArch64 it makes sense to do it at post-legalizer
combining, but *after* the actual combiner loop runs since we don't want it eliminated
before any constant folding runs.

This change by itself has very minor improvements in -Os CTMark overall. What this
does allow is better optimizations when future combines are added that rely on having
expensive constants remain unfolded.

[arsenm]

Seems like this is another one of those DAG workaround passes we ideally wouldn't need to keep running

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll

29

Also add some vector cases?

[aemerson]

In D150179#4328938, @arsenm wrote:

Seems like this is another one of those DAG workaround passes we ideally wouldn't need to keep running

I thought so too, but reading its documentation more I think the problem it's solving is actually agnostic to selectors. That is, the core issue isn't that DAG can only see basic blocks, but one of balancing CSE & register pressure. We could stop running this IR pass (which does exist as IR due to SDAG's nature), but we'd just need to implement something similar in the GISel pipeline anyway.

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll

29

Good point, constant hoisting doesn't touch vector constants so we can avoid this bitcast trickery for those.

[aemerson]

Didn't notice the test failures before. One of them is the verifier complaining about the types being the same for G_BITCAST, so I've relaxed that restriction.

[Pierre-vh]

Just wondering: if we have to add some "special" bitcast that needs to be removed before ISel anyway (+ relax G_BITCAST check rules on top of it), couldn't we just add a dedicated opcode instead?
I'm thinking of something like a G_OPAQUE opcode that serves as an optimization hint that an instruction should be considered opaque and no combiner should attempt to see through it. It could just be removed right before ISel.

I'm wondering if we'll see more use cases for this "hack" later and eventually end up with a use-case that cannot use G_BITCAST (e.g. pointer type?), so we end up doing a similar hack but on another opcode for a different type, etc

For instance, a G_OPAQUE instruction like that could be used to prevent infinite combine loop between the target-independent and target combine rules. If the target combine wants to create a pattern that isn't considered optimal by the generic combiner, it could use that opaque instruction to prevent further optimizations.

[aemerson]

In D150179#4349172, @Pierre-vh wrote:

Just wondering: if we have to add some "special" bitcast that needs to be removed before ISel anyway (+ relax G_BITCAST check rules on top of it), couldn't we just add a dedicated opcode instead?
I'm thinking of something like a G_OPAQUE opcode that serves as an optimization hint that an instruction should be considered opaque and no combiner should attempt to see through it. It could just be removed right before ISel.

I'm wondering if we'll see more use cases for this "hack" later and eventually end up with a use-case that cannot use G_BITCAST (e.g. pointer type?), so we end up doing a similar hack but on another opcode for a different type, etc

For instance, a G_OPAQUE instruction like that could be used to prevent infinite combine loop between the target-independent and target combine rules. If the target combine wants to create a pattern that isn't considered optimal by the generic combiner, it could use that opaque instruction to prevent further optimizations.

Yeah, I think that's ok too.

[arsenm]

In D150179#4357513, @aemerson wrote:

For instance, a G_OPAQUE instruction like that could be used to prevent infinite combine loop between the target-independent and target combine rules. If the target combine wants to create a pattern that isn't considered optimal by the generic combiner, it could use that opaque instruction to prevent further optimizations.

Yeah, I think that's ok too.

Isn't this what the DAG does? There are opaque constants

[aemerson]

In D150179#4363870, @arsenm wrote:

In D150179#4357513, @aemerson wrote:

For instance, a G_OPAQUE instruction like that could be used to prevent infinite combine loop between the target-independent and target combine rules. If the target combine wants to create a pattern that isn't considered optimal by the generic combiner, it could use that opaque instruction to prevent further optimizations.

Yeah, I think that's ok too.

Isn't this what the DAG does? There are opaque constants

Pretty much, except Pierre’s suggestion is more of a dedicated barrier instruction (sort of like freeze), whereas OpaqueConstants are just normal constants with a flag bit set. I think a dedicated barrier is better since it’s messy to have two different constant representations in the MIR.

[aemerson]

I've implemented the separate instruction approach in D151945

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

bool IRTranslator::translateCopy(const User &U, const Value &V,
   }
   return true;
 }
 
 bool IRTranslator::translateBitCast(const User &U,
                                     MachineIRBuilder &MIRBuilder) {
   // If we're bitcasting to the source type, we can reuse the source vreg.
   if (getLLTForType(*U.getOperand(0)->getType(), *DL) ==
-      getLLTForType(*U.getType(), *DL))
+      getLLTForType(*U.getType(), *DL)) {
+    // If the source is a ConstantInt then it was probably created by
+    // ConstantHoisting and we should leave it alone.
+    if (isa<ConstantInt>(U.getOperand(0)))
+      return translateCast(TargetOpcode::G_BITCAST, U, MIRBuilder);
     return translateCopy(U, *U.getOperand(0), MIRBuilder);
+  }
 
   return translateCast(TargetOpcode::G_BITCAST, U, MIRBuilder);
 }
 
 bool IRTranslator::translateCast(unsigned Opcode, const User &U,
                                  MachineIRBuilder &MIRBuilder) {
   Register Op = getOrCreateVReg(*U.getOperand(0));
   Register Res = getOrCreateVReg(U);

llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerCombiner.cpp

 /// AArch64PostLegalizerLowering.
 ///
 /// Combines which don't rely on instruction legality should go in the
 /// AArch64PreLegalizerCombiner.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "AArch64TargetMachine.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/GlobalISel/CSEInfo.h"
 #include "llvm/CodeGen/GlobalISel/Combiner.h"
 #include "llvm/CodeGen/GlobalISel/CombinerHelper.h"
 #include "llvm/CodeGen/GlobalISel/CombinerInfo.h"
 #include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/GISelKnownBits.h"
 #include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
 #include "llvm/CodeGen/GlobalISel/MIPatternMatch.h"
bool AArch64PostLegalizerCombiner::runOnMachineFunction(MachineFunction &MF) {
   MachineDominatorTree *MDT =
       IsOptNone ? nullptr : &getAnalysis<MachineDominatorTree>();
   AArch64PostLegalizerCombinerInfo PCInfo(EnableOpt, F.hasOptSize(),
                                           F.hasMinSize(), KB, MDT);
   GISelCSEAnalysisWrapper &Wrapper =
       getAnalysis<GISelCSEAnalysisWrapperPass>().getCSEWrapper();
   auto *CSEInfo = &Wrapper.get(TPC->getCSEConfig());
   Combiner C(PCInfo, TPC);
-  return C.combineMachineInstrs(MF, CSEInfo);
+  bool Changed = C.combineMachineInstrs(MF, CSEInfo);
+
+  auto &MRI = MF.getRegInfo();
+  // Eliminate any no-op bitcasts that we created to preserve hoisted constants.
+  // We wait until all the combiners have run otherwise the constants may get
+  // re-folded with others during the main combiner loop.
+  for (auto &MBB : MF) {
+    for (auto &MI : make_early_inc_range(MBB)) {
+      if (MI.getOpcode() == TargetOpcode::G_BITCAST) {
+        Register Dst = MI.getOperand(0).getReg();
+        Register Src = MI.getOperand(1).getReg();
+        if (MRI.getType(Dst) != MRI.getType(Src))
+          continue;
+        MRI.replaceRegWith(Dst, Src);
+        MI.eraseFromParent();
+        Changed = true;
+      }
+    }
+  }
+
+  return Changed;
 }
 
 char AArch64PostLegalizerCombiner::ID = 0;
 INITIALIZE_PASS_BEGIN(AArch64PostLegalizerCombiner, DEBUG_TYPE,
                       "Combine AArch64 MachineInstrs after legalization", false,
                       false)
 INITIALIZE_PASS_DEPENDENCY(TargetPassConfig)
 INITIALIZE_PASS_DEPENDENCY(GISelKnownBitsAnalysis)

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mtriple=aarch64-apple-ios -global-isel -stop-after=irtranslator %s -o - | FileCheck %s --check-prefix=TRANSLATED
+; RUN: llc -mtriple=aarch64-apple-ios -global-isel -stop-after=aarch64-postlegalizer-combiner %s -o - | FileCheck %s --check-prefix=PRESELECTION
+
+; Check we don't elide no-op bitcasts of constants since they're used by constant
+; hoisting to prevent constant folding/propagation.
+
+define i32 @test(i32 %a) {
+  ; TRANSLATED-LABEL: name: test
+  ; TRANSLATED: bb.1.entry:
+  ; TRANSLATED-NEXT:   liveins: $w0
+  ; TRANSLATED-NEXT: {{  $}}
+  ; TRANSLATED-NEXT:   [[COPY:%[0-9]+]]:_(s32) = COPY $w0
+  ; TRANSLATED-NEXT:   [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 100000
+  ; TRANSLATED-NEXT:   [[BITCAST:%[0-9]+]]:_(s32) = G_BITCAST [[C]](s32)
+  ; TRANSLATED-NEXT:   [[ADD:%[0-9]+]]:_(s32) = G_ADD [[COPY]], [[BITCAST]]
+  ; TRANSLATED-NEXT:   $w0 = COPY [[ADD]](s32)
+  ; TRANSLATED-NEXT:   RET_ReallyLR implicit $w0
+  ; PRESELECTION-LABEL: name: test
+  ; PRESELECTION: bb.1.entry:
+  ; PRESELECTION-NEXT:   liveins: $w0
+  ; PRESELECTION-NEXT: {{  $}}
+  ; PRESELECTION-NEXT:   [[COPY:%[0-9]+]]:_(s32) = COPY $w0
+  ; PRESELECTION-NEXT:   [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 100000
+  ; PRESELECTION-NEXT:   [[ADD:%[0-9]+]]:_(s32) = G_ADD [[COPY]], [[C]]
+  ; PRESELECTION-NEXT:   $w0 = COPY [[ADD]](s32)
+  ; PRESELECTION-NEXT:   RET_ReallyLR implicit $w0
+entry:
+  %hc = bitcast i32 100000 to i32
+  br label %cont
+cont:
+  %add = add i32 %a, %hc
+  ret i32 %add
+}
+