Page MenuHomePhabricator

[SimplifyCFG] Update passingValueIsAlwaysUndefined to check more attributes
ClosedPublic

Authored by aqjune on Feb 22 2021, 6:48 PM.

Details

Summary

This is a simple patch to update SimplifyCFG's passingValueIsAlwaysUndefined to inspect more attributes.

A new function CallBase::isPassingUndefUB checks attributes that imply noundef.

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 windows > LLVM.CodeGen/AArch64/GlobalISel::postlegalizer-lowering-shuf-to-ins.mir
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -mtriple=aarch64 -run-pass=aarch64-postlegalizer-lowering --aarch64postlegalizerloweringhelper-only-enable-rule="shuf_to_ins" -verify-machineinstrs C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\GlobalISel\postlegalizer-lowering-shuf-to-ins.mir -o - | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\GlobalISel\postlegalizer-lowering-shuf-to-ins.mir

Event Timeline

aqjune created this revision.Feb 22 2021, 6:48 PM
aqjune requested review of this revision.Feb 22 2021, 6:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 6:48 PM

Can you add some tests with deref/derefornull attributes?

aqjune updated this revision to Diff 325696.Feb 23 2021, 12:34 AM

Add tests

@spatel Now I fully got what https://reviews.llvm.org/D96663#2578814 meant. This change is introducing assume because it introduces unreachable which finally turns into assume.

But, an ongoing clang frontend patch will introduce many more noundefs, and this will anyway create assumes;
If the assumes inserted by this patch cause problems, it is likely that the problems will happen with the noundef patch as well. I think it is about facing the problem earlier or not.

If we decide to be lightweight (and not to insert assume), I'll move towards SimplifyCFG to simply swap the phi's operand with undef if nonnull without noundef is met.

@spatel Now I fully got what https://reviews.llvm.org/D96663#2578814 meant. This change is introducing assume because it introduces unreachable which finally turns into assume.

But, an ongoing clang frontend patch will introduce many more noundefs, and this will anyway create assumes;
If the assumes inserted by this patch cause problems, it is likely that the problems will happen with the noundef patch as well. I think it is about facing the problem earlier or not.

If we decide to be lightweight (and not to insert assume), I'll move towards SimplifyCFG to simply swap the phi's operand with undef if nonnull without noundef is met.

Another idea - proposal to limit the creation of assumes from simplifycfg:
D97306

spatel accepted this revision.Feb 23 2021, 11:44 AM

LGTM. As with D61409, we want to watch for regressions, but I don't think we need to delay. If there are problems, we can try to use assume bundles or other means as suggested D97306.

This revision is now accepted and ready to land.Feb 23 2021, 11:44 AM

Okay, thanks..!

Carrot added a subscriber: Carrot.Mar 16 2021, 5:17 PM
Carrot added inline comments.
llvm/include/llvm/IR/InstrTypes.h
1663

It is not true for "this" parameter. It always has dereferenceable and nonnull attribute, if "this" is not used in the function, passing in undef is no problem.

It causes wrong code generated for our application.

aqjune added inline comments.Mar 16 2021, 5:23 PM
llvm/include/llvm/IR/InstrTypes.h
1663

passing in undef is no problem

This property holds for nonnull and align, but not for the three attributes listed here. Passing undef to these is UB because LangRef says violation of these is UB. dereferenceable paragraph says:

The pointer should be well defined, otherwise it is undefined behavior

Is your program being transformed from passing valid this to passing undef by some pass? Then, the pass is the buggy one.

Carrot added inline comments.Mar 16 2021, 6:04 PM
llvm/include/llvm/IR/InstrTypes.h
1663

Since "this" is not used in the function, DeadArgumentElimination pass changes all callers' valid "this" to undef.

@spatel, what's your opinion on where to fix it?

aqjune added inline comments.Mar 16 2021, 6:08 PM
llvm/include/llvm/IR/InstrTypes.h
1663

Since "this" is not used in the function, DeadArgumentElimination pass changes all callers' valid "this" to undef.

Then DeadArgumentElimination should drop attributes like dereferenceable, etc from the argument definition I think, but reviewers may have different ideas about this.