Skip to content

Conversation

@resistor
Copy link
Collaborator

@resistor resistor commented Dec 4, 2024

This does not impact any in-tree targets, but does impact CHERI targets.

…largest legal integers.

This does not impact any in-tree targets, but does impact CHERI targets.
@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Owen Anderson (resistor)

Changes

This does not impact any in-tree targets, but does impact CHERI targets.


Full diff: https://github.com/llvm/llvm-project/pull/118584.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/VNCoercion.cpp (+5)
diff --git a/llvm/lib/Transforms/Utils/VNCoercion.cpp b/llvm/lib/Transforms/Utils/VNCoercion.cpp
index 1e0ae280516410..7fed5121831338 100644
--- a/llvm/lib/Transforms/Utils/VNCoercion.cpp
+++ b/llvm/lib/Transforms/Utils/VNCoercion.cpp
@@ -248,6 +248,11 @@ int analyzeLoadFromClobberingMemInst(Type *LoadTy, Value *LoadPtr,
   ConstantInt *SizeCst = dyn_cast<ConstantInt>(MI->getLength());
   if (!SizeCst)
     return -1;
+  // If this is a pointer type that's larger than the largest integer that we
+  // support, then ignore it.
+  if (LoadTy->isPointerTy() &&
+      DL.getTypeSizeInBits(LoadTy) > DL.getLargestLegalIntTypeSizeInBits())
+    return -1;
   uint64_t MemSizeInBits = SizeCst->getZExtValue() * 8;
 
   // If this is memset, we just need to see if the offset is valid in the size

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test. This should be possible by just specifying an appropriate data layout, even outside the CHERI fork.

However, I don't really understand why this condition is necessary at all. How is the largest legal integer type relevant in this context?

@resistor resistor marked this pull request as draft December 4, 2024 11:09
@resistor
Copy link
Collaborator Author

resistor commented Dec 4, 2024

Moving this back to draft for now.

The origins of this code in the CHERI downstream of LLVM is somewhat mysterious (it first appears in an old merge commit, without a test, and then has gotten heavily refactored in a later merge with upstream that extracted code from GVN.cpp to VNCoercion.cpp).

I believe the original intent was to stop GVN from trying to use non-integral pointer types for VN coercion. Looking at the current code canCoerceMustAliasedValueToLoad, I suspect this is no longer needed, and might in fact already be bit-rotted.

@resistor resistor closed this Dec 4, 2024
@resistor
Copy link
Collaborator Author

Tagging @jrtc27 and @arichardson for awareness on CHERI-derived PRs

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 10, 2024

We know we have issues with GVN not honouring pointer provenance in ways that matter once that provenance is captured in hardware, whether via CHERI or some other provenance-tracking scheme that doesn't expose the provenance as bits in the address you can mess with. There's no sense upstreaming random historical cruft that we know is not sufficient, and may not be necessary.

@resistor
Copy link
Collaborator Author

We know we have issues with GVN not honouring pointer provenance in ways that matter once that provenance is captured in hardware, whether via CHERI or some other provenance-tracking scheme that doesn't expose the provenance as bits in the address you can mess with. There's no sense upstreaming random historical cruft that we know is not sufficient, and may not be necessary.

Agreed, I've abandoned this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants