[META][GVN] NewGVN Integration

[Quuxplusone]

Bugzilla Link	PR30995
Status	NEW
Importance	P normal
Reported by	Simon Pilgrim (llvm-dev@redking.me.uk)
Reported on	2016-11-12 06:04:48 -0800
Last modified on	2021-11-05 16:25:03 -0700
Version	trunk
Hardware	PC All
CC	alina.sbirlea@gmail.com, andrew@ziglang.org, bigcheesegs@gmail.com, ditaliano@apple.com, fedor.v.sergeev@gmail.com, filcab@gmail.com, florian_hahn@apple.com, geek4civic@gmail.com, hfinkel@anl.gov, lebedev.ri@gmail.com, llvm-bugs@lists.llvm.org, lukebenes@hotmail.com, nunoplopes@sapo.pt, paulsson@linux.vnet.ibm.com, sanjoy@playingwithpointers.com, simon.f.whittaker@gmail.com, stephan.bergmann.secondary@googlemail.com
Fixed by commit(s)
Attachments	newgvn_fail.ll (63518 bytes, application/octet-stream)
Blocks
Blocked by	PR30771, PR31511, PR31523, PR31613, PR31792, PR31867, PR32446, PR36335, PR37121, PR30550, PR31468, PR31472, PR31480, PR31483, PR31491, PR31499, PR31501, PR31512, PR31554, PR31573, PR31594, PR31682, PR31686, PR31698, PR31749, PR31758, PR31868, PR32349, PR32403, PR33165, PR33367, PR33400, PR35074, PR35189, PR35583, PR35801, PR35839, PR37540, PR37541, PR39056, PR39057, PR39279, PR42422
See also	PR25321

Meta ticket covering the addition of the NewGVN pass.

WIP Patch: https://reviews.llvm.org/D26224

[Quuxplusone]

The initial support was merged upstream. I'll keep this open for a while as an umbrella for all the NewGVN issues.

[Quuxplusone]

FAIL: "The instructions for these memory operations should have been in the same congruence class"

You are probably already aware of this, but the test-suite fails with '-mllvm -enable-newgvn'. I am posting this here for other users sake who might also follow Davide's suggestion and run the test-suite on some target.

I ran the test-suite on SystemZ with NewGVN, and found then this crash. I then checked on X86 to also find the same fail there.

test-suite/MultiSource/Benchmarks/mafft/io.c:

clang-4.0: /home/jonas/llvm/llvm-dev/lib/Transforms/Scalar/NewGVN.cpp:1427: void NewGVN::verifyMemoryCongruency(): Assertion `ValueToClass.lookup(FirstMUD->getMemoryInst()) == ValueToClass.lookup(SecondMUD->getMemoryInst()) && "The instructions for these memory operations should have been in " "the same congruence class"' failed.

I used llvm/trunk@290872, cfe/trunk@290887 and test-suite/trunk@290894.

Options: -O3 -mllvm -enable-newgvn

/Jonas

[Quuxplusone]

Thanks Jonas, going to reduce/debug this soon =)

[Quuxplusone]

Jonas, do you mind to try ToT? Danny fixed all the remaining unknown issues.

[Quuxplusone]

(In reply to comment #4)
> Jonas, do you mind to try ToT? Danny fixed all the remaining unknown issues.

*known, even.

[Quuxplusone]

Sorry, but I haven't even opened the NewGVN.cpp file. My idea was to contribute by running the test-suite on SystemZ, which I did. I was expecting the author(s) of the code would then take the time to fix any issues.

[Quuxplusone]

(and yes, the crash I found is still there as far as I can see)

[Quuxplusone]

(In reply to comment #6)
> Sorry, but I haven't even opened the NewGVN.cpp file. My idea was to
> contribute by running the test-suite on SystemZ, which I did. I was
> expecting the author(s) of the code would then take the time to fix any
> issues.

I did =) https://llvm.org/bugs/show_bug.cgi?id=31573

[Quuxplusone]

Ah, great :-)

I tried to apply the attached patch, but it did not apply cleanly anymore, at least not in my repo with latest changes.

Let me know when the test-suite passes on X86, and I will gladly run it on SystemZ.

/Jonas

[Quuxplusone]

(In reply to comment #9)
> Ah, great :-)
>
> I tried to apply the attached patch, but it did not apply cleanly anymore,
> at least not in my repo with latest changes.
>
> Let me know when the test-suite passes on X86, and I will gladly run it on
> SystemZ.
>
> /Jonas

LLVM passes test-suite on X86 now, so I think we're in a good shape to try on
other platforms if you want to take a look.

[Quuxplusone]

(In reply to comment #10)
> (In reply to comment #9)
> > Ah, great :-)
> >
> > I tried to apply the attached patch, but it did not apply cleanly anymore,
> > at least not in my repo with latest changes.
> >
> > Let me know when the test-suite passes on X86, and I will gladly run it on
> > SystemZ.
> >
> > /Jonas
>
> LLVM passes test-suite on X86 now, so I think we're in a good shape to try
> on other platforms if you want to take a look.

damn, s/LLVM/LLVM with NewGVN/

[Quuxplusone]

Attached newgvn_fail.ll (63518 bytes, application/octet-stream): reduced test case for opt with newgvn crash

[Quuxplusone]

Huh, I got still another fail today on SystemZ with
llvm/trunk@292353
cfe/trunk@292343
test-suite/trunk@292129

NewGVN.cpp:1084: void NewGVN::moveValueToNewCongruenceClass(llvm::Instruction*,
CongruenceClass*, CongruenceClass*): Assertion `(!isa<Instruction>(NewClass-
>RepLeader) || !NewClass->RepLeader || I == NewClass->RepLeader || !DT-
>properlyDominates( I->getParent(), cast<Instruction>(NewClass->RepLeader)-
>getParent())) && "New class for instruction should not be dominated by
instruction"' failed.

/bin/opt -O3 -enable-newgvn -S ./newgvn_fail.ll

I postedthe reduced test-case above.

[Quuxplusone]

(In reply to comment #13)
> Huh, I got still another fail today on SystemZ with
> llvm/trunk@292353
> cfe/trunk@292343
> test-suite/trunk@292129
>
> NewGVN.cpp:1084: void
> NewGVN::moveValueToNewCongruenceClass(llvm::Instruction*, CongruenceClass*,
> CongruenceClass*): Assertion `(!isa<Instruction>(NewClass->RepLeader) ||
> !NewClass->RepLeader || I == NewClass->RepLeader || !DT->properlyDominates(
> I->getParent(), cast<Instruction>(NewClass->RepLeader)->getParent())) &&
> "New class for instruction should not be dominated by instruction"' failed.
>
> /bin/opt -O3 -enable-newgvn -S ./newgvn_fail.ll
>
> I postedthe reduced test-case above.

Thank you very much. I'll reduce/investigate this. For the future, do you mind
to open a new issue and link it here? It would make our tracking much easier =)