Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm-diff] Also diff alloca's allocated type and alignment #84781

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

YanWQ-monad
Copy link
Contributor

Currently, the function

// If AllowAssumptions is enabled, whenever we encounter a pair of values
// that we cannot prove to be equivalent, we assume equivalence and store that
// assumption to be checked later in BlockDiffCandidates.
bool diff(const Instruction *L, const Instruction *R, bool Complain,
bool TryUnify, bool AllowAssumptions) {
// FIXME: metadata (if Complain is set)

doesn't handle the non-operand data of AllocaInst. For example,

alloca i64, align 1
alloca i32, align 1

would be considered the same instruction.


Under some special circumstances, that would make llvm-diff crash:

define ptr @func() {
  %1 = alloca i64, align 1
  %2 = alloca i32, align 1
  %3 = alloca i8, align 1
  ret ptr %2
}
define ptr @func() {
  %1 = alloca i64, align 1
  %2 = alloca i32, align 1
  ret ptr %2
}
$ ./build/bin/llvm-diff lhs.ll rhs.ll
in function func:
  in block %0 / %0:
    in instruction   ret ptr %2 /   ret ptr %2:
      operands %2 and %2 differ
llvm-diff: path/llvm/tools/llvm-diff/lib/DifferenceEngine.cpp:268: void (anonymous namespace)::FunctionDifferenceEngine::unify(const Instruction *, const Instruction *): Assertion `!Result && "structural differences second time around?"' failed.
fish: Job 1, './build/bin/llvm-diff lhs.ll r…' terminated by signal SIGABRT (Abort)

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@dtcxzyw dtcxzyw requested a review from nikic March 11, 2024 16:18
@dtcxzyw dtcxzyw added the llvm-tools All llvm tools that do not have corresponding tag label Mar 11, 2024
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.

This looks correct, but I think it only scratches the surface of the problem? The same problem exists for load types, GEP types, and basically everything else that is checked by hasSameSpecialState(). Can we use that instead?

@YanWQ-monad
Copy link
Contributor Author

Adding test case

@YanWQ-monad
Copy link
Contributor Author

YanWQ-monad commented Mar 11, 2024

When I was adding the test cases, I found another weird thing: same struct in different .lls seems to be considered different types, although in the same LLVMContext. This can also be seen from the failed tests.

I'm gonna investigate this issue tomorrow, because I have to go sleep now - I have lessons tomorrow.

@YanWQ-monad
Copy link
Contributor Author

After investigation, I found that two struct declarations always have different Type pointers, despite they have the same element.

The related code might be

StructType *StructType::create(LLVMContext &Context, StringRef Name) {
StructType *ST = new (Context.pImpl->Alloc) StructType(Context);
if (!Name.empty())
ST->setName(Name);
return ST;
}
StructType *StructType::get(LLVMContext &Context, bool isPacked) {
return get(Context, std::nullopt, isPacked);
}
StructType *StructType::create(LLVMContext &Context, ArrayRef<Type*> Elements,
StringRef Name, bool isPacked) {
StructType *ST = create(Context, Name);
ST->setBody(Elements, isPacked);
return ST;
}

We can infer that different calls to StructType::create will always create different Type*s, which cause llvm-diff (of the current PR) to consider same struct as different types.

So, should we modify the code in Type.cpp to make it generate same pointer for structure with same element, or handle this situation in llvm-diff in a special way so that it won't report the differences?


Reproduction (llvm-diff of the current PR):

%struct = type { i8 }

define void @gep(ptr %addr) {
  %1 = getelementptr %struct, ptr %addr, i32 0
  ret void
}

Command: llvm-diff a.ll a.ll, results in

in function gep:
  in block %0 / %0:
    >   %1 = getelementptr %struct.0, ptr %addr, i32 0
    <   %1 = getelementptr %struct, ptr %addr, i32 0

@YanWQ-monad
Copy link
Contributor Author

TL;DR: added a function isSameType(Type *TL, Type *TR) to check whether TL on the left side and TR on the right side are structural equivalence. It recursively compares the inner elements of StructType, VectorType, and ArrayType, and caches the result in IsSameTypes.

In diff(Instruction *L, Instruction *R, .....) function, for type-related instructions (load, gep, alloca), check their type equivalence using isSameType; and for other instructions, it calls hasSameSpecialState to compare other non-operand data.

@YanWQ-monad
Copy link
Contributor Author

ping?

@YanWQ-monad
Copy link
Contributor Author

ping @nikic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm-tools All llvm tools that do not have corresponding tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants