Modify DXILPrepare to emit no-op bitcasts

[beanz]

Summary

In supporting opaque pointers we need to re-materialize typed pointers
in bitcode emission. Because of how the value-enumerator pre-allocates
types and instructions we need to insert some no-op bitcasts in the
places that we'll need bitcasts for the pointer types.

[beanz]

Updateing DXIL-prepare to rely on dxil::TypedPointerType, and to properly handle function types.

[beanz]

Rebasing on updated patches from earlier in the stack.

[python3kgae]

llvm/lib/Target/DirectX/DXILPrepare.cpp

141

Why bitcasts need to be at top of the block?
Could there be phi at top of the block?

154

Cannot find the check for only have one type.
Does it hide in PointerTypes.find ?

155–160

Could we have shouldOmit function and createNoOpBitcast function? The same code happened 3 times.

[beanz]

llvm/lib/Target/DirectX/DXILPrepare.cpp

141

Good point. I will adjust.

154

That comment isn't great. The pointer analysis assigns one type to each value based on how it is used. If the type of the user and the type assigned by pointer analysis match, we don't need a bitcast because the value will be assigned that type. If they don't match, we need a bitcast so that the user gets the correct typed pointer.

[beanz]

llvm/lib/Target/DirectX/DXILPrepare.cpp

155–160

The code here is actually not the same in all three places. The first two are super similar except that Inst is different types in both cases. The third is different because the interfaces for GEP instructions aren't the same.

[beanz]

Updates based on PR feedback.

[beanz]

Fixing a code comment.

[kuhar]

llvm/lib/Target/DirectX/DXILPrepare.cpp

105–106

nit: If you want to revisit this in a future patch, I think you can use enum_seq here: https://llvm.org/doxygen/Sequence_8h.html

113

I know this is existing code, but a tiny nit: I think the coding style prefers the end value to be calculated only once, e.g., for (size_t i = 0, e = size(); i != e; ++i) ...

127

nit: missing . at the end of the comment

133

Since I is a reference, it cannot be null, so we should use plain dyn_cast.
Second, the type on the left should be auto *.
Last, could we give it a more descriptive name since we know the type? Maybe Load or LI.

134

nit: missing . at the end of the comment

135–136

It is shadowed by other iterator variables below. Can we give it a more descriptive/unique name?

151

same here

152

also here

168

same here

169

also here

170–179

This pattern seems pretty repetitive: for each type, we check if the pointer operand is in the type, adjust the inter point, and emit a bitcast. Could we move it to a generic helper function? Something like:

if (Value *Bitcast = createNoopBitcast(Inst, Inst->getPointerOperand(), Inst->getPointerOperandType()) {
  // Update or replace Inst.
  continue;
}

[kuhar]

llvm/test/CodeGen/DirectX/omit-bitcast-insert.ll

26

I think it would be also worth having some tests where we do insert bitcasts for geps.

[beanz]

Updating based on PR feedback. Thank's @kuhar!

llvm/lib/Target/DirectX/DXILPrepare.cpp

 //===- DXILPrepare.cpp - Prepare LLVM Module for DXIL encoding ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file This file contains pases and utilities to convert a modern LLVM
 /// module into a module compatible with the LLVM 3.7-based DirectX Intermediate
 /// Language (DXIL).
 //===----------------------------------------------------------------------===//
 
 #include "DirectX.h"
+#include "PointerTypeAnalysis.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Compiler.h"
 
 #define DEBUG_TYPE "dxil-prepare"
 
 using namespace llvm;
+using namespace llvm::dxil;
 
 namespace {
 
 constexpr bool isValidForDXIL(Attribute::AttrKind Attr) {
   return is_contained({Attribute::Alignment,
                        Attribute::AlwaysInline,
                        Attribute::Builtin,
                        Attribute::ByVal,
return is_contained({Attribute::Alignment,
                        Attribute::SanitizeThread,
                        Attribute::SanitizeMemory,
                        Attribute::UWTable,
                        Attribute::ZExt},
                       Attr);
 }
 
 class DXILPrepareModule : public ModulePass {
+
+  static Value *maybeGenerateBitcast(IRBuilder<> &Builder,
+                                     PointerTypeMap &PointerTypes,
+                                     Instruction &Inst, Value *Operand,
+                                     Type *Ty) {
+    // Omit bitcasts if the incoming value matches the instruction type.
+    auto It = PointerTypes.find(Operand);
+    if (It != PointerTypes.end())
+      if (cast<TypedPointerType>(It->second)->getElementType() == Ty)
+        return nullptr;
+    // Insert bitcasts where we are removing the instruction.
+    Builder.SetInsertPoint(&Inst);
+    // This code only gets hit in opaque-pointer mode, so the type of the
+    // pointer doesn't matter.
+    return Builder.Insert(CastInst::Create(Instruction::BitCast, Operand,
+                                           Builder.getInt8PtrTy()));
+  }
+
 public:
   bool runOnModule(Module &M) override {
+    PointerTypeMap PointerTypes = PointerTypeAnalysis::run(M);
     AttributeMask AttrMask;
     for (Attribute::AttrKind I = Attribute::None; I != Attribute::EndAttrKinds;
          I = Attribute::AttrKind(I + 1)) {
       if (!isValidForDXIL(I))
         AttrMask.addAttribute(I);
     }
     for (auto &F : M.functions()) {
       F.removeFnAttrs(AttrMask);
       F.removeRetAttrs(AttrMask);
-      for (size_t Idx = 0; Idx < F.arg_size(); ++Idx)
+      for (size_t Idx = 0, End = F.arg_size(); Idx < End; ++Idx)
         F.removeParamAttrs(Idx, AttrMask);
 
       for (auto &BB : F) {
         IRBuilder<> Builder(&BB);
         for (auto &I : make_early_inc_range(BB)) {
           if (I.getOpcode() == Instruction::FNeg) {
             Builder.SetInsertPoint(&I);
             Value *In = I.getOperand(0);
             Value *Zero = ConstantFP::get(In->getType(), -0.0);
             I.replaceAllUsesWith(Builder.CreateFSub(Zero, In));
             I.eraseFromParent();
+            continue;
+          }
+          // Only insert bitcasts if the IR is using opaque pointers.
+          if (!M.getContext().hasSetOpaquePointersValue())
+            continue;
+
+          // Emtting NoOp bitcast instructions allows the ValueEnumerator to be
+          // unmodified as it reserves instruction IDs during contruction.
+          if (auto LI = dyn_cast<LoadInst>(&I)) {
+            if (Value *NoOpBitcast = maybeGenerateBitcast(
+                    Builder, PointerTypes, I, LI->getPointerOperand(),
+                    LI->getType())) {
+              LI->replaceAllUsesWith(
+                  Builder.CreateLoad(LI->getType(), NoOpBitcast));
+              LI->eraseFromParent();
+            }
+            continue;
+          }
+          if (auto SI = dyn_cast<StoreInst>(&I)) {
+            if (Value *NoOpBitcast = maybeGenerateBitcast(
+                    Builder, PointerTypes, I, SI->getPointerOperand(),
+                    SI->getValueOperand()->getType())) {
+
+              SI->replaceAllUsesWith(
+                  Builder.CreateStore(SI->getValueOperand(), NoOpBitcast));
+              SI->eraseFromParent();
+            }
+            continue;
+          }
+          if (auto GEP = dyn_cast<GetElementPtrInst>(&I)) {
+            if (Value *NoOpBitcast = maybeGenerateBitcast(
+                    Builder, PointerTypes, I, GEP->getPointerOperand(),
+                    GEP->getResultElementType()))
+              GEP->setOperand(0, NoOpBitcast);
+            continue;
           }
         }
       }
     }
     return true;
   }
 
   DXILPrepareModule() : ModulePass(ID) {}
 
   static char ID; // Pass identification.
 };
 char DXILPrepareModule::ID = 0;
 
 } // end anonymous namespace
 
 INITIALIZE_PASS_BEGIN(DXILPrepareModule, DEBUG_TYPE, "DXIL Prepare Module",
                       false, false)
 INITIALIZE_PASS_END(DXILPrepareModule, DEBUG_TYPE, "DXIL Prepare Module", false,
                     false)
 
 ModulePass *llvm::createDXILPrepareModulePass() {
   return new DXILPrepareModule();
 }

llvm/test/CodeGen/DirectX/conflicting-bitcast-insert.ll

+; RUN: llc --filetype=asm %s -o - | FileCheck %s
+target triple = "dxil-unknown-unknown"
+
+define i64 @test(ptr %p) {
+  store i32 0, ptr %p
+  %v = load i64, ptr %p
+  ret i64 %v
+}
+
+; CHECK: define i64 @test(ptr %p) {
+; CHECK-NEXT: %1 = bitcast ptr %p to ptr
+; CHECK-NEXT: store i32 0, ptr %1, align 4
+; CHECK-NEXT: %2 = bitcast ptr %p to ptr
+; CHECK-NEXT: %3 = load i64, ptr %2, align 8
+
+define i64 @testGEP(ptr %p) {
+  %ptr = getelementptr i32, ptr %p, i32 4
+  %val = load i64, ptr %p
+  ret i64 %val
+}
+
+; CHECK: define i64 @testGEP(ptr %p) {
+; CHECK-NEXT:   %1 = bitcast ptr %p to ptr
+; CHECK-NEXT:   %ptr = getelementptr i32, ptr %1, i32 4
+; CHECK-NEXT:   %2 = bitcast ptr %p to ptr
+; CHECK-NEXT:   %3 = load i64, ptr %2, align 8

llvm/test/CodeGen/DirectX/omit-bitcast-insert.ll

+; RUN: llc --filetype=asm %s -o - | FileCheck %s
+target triple = "dxil-unknown-unknown"
+
+define i64 @test(ptr %p) {
+  %v = load i64, ptr %p
+  ret i64 %v
+}
+
+; CHECK: define i64 @test(ptr %p) {
+; CHECK-NEXT: %v = load i64, ptr %p, align 8
+; CHECK-NEXT: ret i64 %v
+
+define i64 @test2(ptr %p) {
+  store i64 0, ptr %p
+  %v = load i64, ptr %p
+  ret i64 %v
+}
+
+; CHECK: define i64 @test2(ptr %p) {
+; CHECK-NEXT: store i64 0, ptr %p
+; CHECK-NEXT: %v = load i64, ptr %p, align 8
+; CHECK-NEXT: ret i64 %v
+
+define i32 @test3(ptr %0)  {
+  %2 = getelementptr i32, ptr %0, i32 4
+  %3 = load i32, ptr %2
+  ret i32 %3
+}
+
+; CHECK: define i32 @test3(ptr %0)  {
+; CHECK-NEXT: %2 = getelementptr i32, ptr %0, i32 4
+; CHECK-NEXT: %3 = load i32, ptr %2