[PATCH] D82680: MSAN: Allow emitting checks for struct types

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1155
     unsigned StoreSize = DL.getTypeStoreSize(Shadow->getType());
     if (Shadow->getType()->isAggregateType()) {
       paintOrigin(IRB, updateOrigin(Origin, IRB), OriginPtr, StoreSize,
----------------
this can be improved now by moving the origin update code behind the shadow check


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1409
+    for (unsigned i = 0; i < Struct->getNumElements(); i++) {
+      Value *ShadowItem = ShadowItem = IRB.CreateExtractValue(Shadow, i);
+      assert(ShadowItem && "Failed to create ExtractValue instruction!");
----------------
extra "ShadowItem ="


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1410
+      Value *ShadowItem = ShadowItem = IRB.CreateExtractValue(Shadow, i);
+      assert(ShadowItem && "Failed to create ExtractValue instruction!");
+      Value *ShadowInner = convertShadowToScalar(ShadowItem, IRB);
----------------
I don't think this is possible


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1413
+      Value *ShadowBool =
+          IRB.CreateICmpNE(ShadowInner, getCleanShadow(ShadowInner));
+      Aggregator = IRB.CreateOr(Aggregator, ShadowBool);
----------------
I'd prefer if this generated less pointless comparisons.

It would be nice if CreateICmpNE(i1 x, i1 false) automatically folded to "x". I think this is a reasonable change to make in IRBuilder, even though this is probably a very unlikely situation.

As an alternative, make  convertShadowToScalar return i1 and remove icmp in the callers (call it something like convertShadowToBool). Check Combiner class how not to emit a "false" constant. As for the Maybe* callers, it's ok to not handle aggregates there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82680