[PATCH] D133283: [DirectX backend] Support global ctor for DXILBitcodeWriter.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:2112
         Record.push_back(VE.getValueID(Op));
+      }
       AbbrevToUse = AggregateAbbrev;
----------------
nit: you added extra braces here.

see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:34
+  } else if (auto *GV = dyn_cast<GlobalVariable>(V))
+    PointeeTy = GV->getValueType();
+
----------------
nit: please add braces here.

This is an example of the contrary point in the standard where readability is impacted by the braces. Since the first `if` has braces to improve readability we should keep braces on all the `else` blocks too.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:83
+      Type *NewRetTy = classifyPointerType(RetInst->getReturnValue());
+      if (!RetTy && !NewRetTy)
+        RetTy = NewRetTy;
----------------
This confuses me. `RetTy` starts as `nullptr`, if `NewRetTy` is also `nullptr` we set `RetTy` to `nullptr`? Unless I'm missing something this does nothing.

This changes the logic in the loop body, but not in a way that is clear to follow.

If I'm reading this logic correctly, I don't see how this function ever returns anything other than `i8*`. You enter the loop `RetTy` is `nullptr`. If `NewRetTy` is anything, you have a mismatch, and get `i8*`.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:107
 
+static void classifyGlobalCtorPointerType(const GlobalVariable &GV,
+                                          PointerTypeMap &Map) {
----------------
Is there a more generic way to do this? I feel like this more or less hard codes the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133283