This is an archive of the discontinued LLVM Phabricator instance.

[NewGVN] add context instruction for SimplifyQuery
ClosedPublic

Authored by bcl5980 on Jun 15 2022, 11:37 PM.

Details

Summary

NewGVN will find operator from other context. ValueTracking currently doesn't have a way to run completely without context instruction.
So it will use operator itself as conext instruction.
If the operator in another branch will never be executed but it has an assume, it may caused value tracking use the assume to do wrong simpilfy.

Check useInstrInfo flag in value tracking, if we haven't set the flag we will not use valjue itself as context instruction to fix the issue.

Fix #56039

Diff Detail

Event Timeline

bcl5980 created this revision.Jun 15 2022, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 11:37 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
bcl5980 requested review of this revision.Jun 15 2022, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 11:37 PM

Could you please explain what's the bug and why is this the right fix?

bcl5980 added a comment.EditedJun 16 2022, 1:32 AM

Could you please explain what's the bug and why is this the right fix?

The min reproduce RI has two load %b at different basic blocks. %bb2 has assumption for load %b data.
When newgvn pass try to call simplifyCmpInst for %tobool3.not.i = icmp eq i8 %lb1, 0, operand 0 is replaced to %lb2 based on allocate BasicExpression code.
%lb2 has assumption lager than 0 cause %tobool3.not.i = icmp eq i8 %lb1, 0 replaced by constant false.
But if the icmp return false %bb2 will be a dead block cause assumption is also dead.
The root cause is NewGVN haven't pass context instruction to simplify function cause wrong transform happen.
So we need to pass context instruction to avoid simplifyCmpInst return false to fix the issue.

nikic added a comment.Jun 16 2022, 1:48 AM

SimplifyContext is created with CxtI=nullptr though, so I would expect it to not use a context instruction. Is the problem here that InstSimplify calls into ValueTracking helps like isKnownNonZero, and those convert CxtI==nullptr into the passed instruction in safeCxtI?

SimplifyContext is created with CxtI=nullptr though, so I would expect it to not use a context instruction. Is the problem here that InstSimplify calls into ValueTracking helps like isKnownNonZero, and those convert CxtI==nullptr into the passed instruction in safeCxtI?

Yes, it call isKnownNonZero to fold %tobool3.not.i = icmp eq i8 %lb1, 0 to false.

nikic accepted this revision.Jun 21 2022, 12:35 AM

LGTM, but please update the patch description with details. I think it would be better to make these simplification queries not use context at all, but that would require some API changes. For now, this does seem to be better than before.

This revision is now accepted and ready to land.Jun 21 2022, 12:35 AM
fhahn added a comment.Jun 21 2022, 2:26 AM

SimplifyContext is created with CxtI=nullptr though, so I would expect it to not use a context instruction. Is the problem here that InstSimplify calls into ValueTracking helps like isKnownNonZero, and those convert CxtI==nullptr into the passed instruction in safeCxtI?

Yes, it call isKnownNonZero to fold %tobool3.not.i = icmp eq i8 %lb1, 0 to false.

I guess I am still not sure where and why a context instruction appears to be used here, as at the moment SQ never sets one and no instruction is passed to simplify*?

It's been a while since I had a close look at the code, but I *think* here we intentionally do not use any context information for instruction simplify (e.g. we also avoid using flags on instructions when looking through operands).Conceptually I think the expressions here are intended to be evaluated independently of their location. If we change that, I am worried that we will trade one bug for another, perhaps more subtle one. It *sounds* like it would be better to adjust the API to do not come up with a different context instruction, if no CxtI is added or UseInstrInfo is false.

nikic added a comment.Jun 21 2022, 2:53 AM

SimplifyContext is created with CxtI=nullptr though, so I would expect it to not use a context instruction. Is the problem here that InstSimplify calls into ValueTracking helps like isKnownNonZero, and those convert CxtI==nullptr into the passed instruction in safeCxtI?

Yes, it call isKnownNonZero to fold %tobool3.not.i = icmp eq i8 %lb1, 0 to false.

I guess I am still not sure where and why a context instruction appears to be used here, as at the moment SQ never sets one and no instruction is passed to simplify*?

See https://github.com/llvm/llvm-project/blob/ae76b2f455016efb8cac5519d382be575b2d2edc/llvm/lib/Analysis/ValueTracking.cpp#L133. ValueTracking currently doesn't have a way to run completely without context instruction.

It's been a while since I had a close look at the code, but I *think* here we intentionally do not use any context information for instruction simplify (e.g. we also avoid using flags on instructions when looking through operands).Conceptually I think the expressions here are intended to be evaluated independently of their location. If we change that, I am worried that we will trade one bug for another, perhaps more subtle one. It *sounds* like it would be better to adjust the API to do not come up with a different context instruction, if no CxtI is added or UseInstrInfo is false.

I'm also somewhat apprehensive here -- the reason why I thought this would be safe is that this is part of the computation a specific instruction -- we don't fold every place where the same expression is used. However, the operands might come from a different context (which is why we also can't UseInstrInfo). In theory, I don't think that should be an issue, but in practice it may well be.

bcl5980 edited the summary of this revision. (Show Details)Jun 21 2022, 9:21 PM
This revision was landed with ongoing or failed builds.Jun 21 2022, 9:25 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 22 2022, 6:42 AM

See https://github.com/llvm/llvm-project/blob/ae76b2f455016efb8cac5519d382be575b2d2edc/llvm/lib/Analysis/ValueTracking.cpp#L133. ValueTracking currently doesn't have a way to run completely without context instruction.

I'm also somewhat apprehensive here -- the reason why I thought this would be safe is that this is part of the computation a specific instruction -- we don't fold every place where the same expression is used. However, the operands might come from a different context (which is why we also can't UseInstrInfo). In theory, I don't think that should be an issue, but in practice it may well be.

Ah right, so it sounds a better fix would be to update ValueTracking to respect UseInstrInfo (only set by NewGVN AFAIK, so we could also rename it/update the documentation) when picking a safe context instruction, by moving the static function to SimplifyQuery?

See https://github.com/llvm/llvm-project/blob/ae76b2f455016efb8cac5519d382be575b2d2edc/llvm/lib/Analysis/ValueTracking.cpp#L133. ValueTracking currently doesn't have a way to run completely without context instruction.

I'm also somewhat apprehensive here -- the reason why I thought this would be safe is that this is part of the computation a specific instruction -- we don't fold every place where the same expression is used. However, the operands might come from a different context (which is why we also can't UseInstrInfo). In theory, I don't think that should be an issue, but in practice it may well be.

Ah right, so it sounds a better fix would be to update ValueTracking to respect UseInstrInfo (only set by NewGVN AFAIK, so we could also rename it/update the documentation) when picking a safe context instruction, by moving the static function to SimplifyQuery?

Do I need to revert current code now ?

bcl5980 updated this revision to Diff 439005.Jun 22 2022, 7:08 AM
bcl5980 edited the summary of this revision. (Show Details)

do not use value as context if UseInstrInfo is false.

@fhahn, is this the better way you say?

fhahn added inline comments.Jun 24 2022, 1:50 AM
llvm/lib/Analysis/ValueTracking.cpp
134

it seems it would be slightly simpler to move safeCxtI to SimplifyQuery?

fhahn added a comment.Jun 24 2022, 1:51 AM

Also, it would probably be good to have this patch as a separate one, with an updated title/description

Also, it would probably be good to have this patch as a separate one, with an updated title/description

OK. I will try to work on it on another patch.

bcl5980 added inline comments.Jun 24 2022, 9:38 AM
llvm/lib/Analysis/ValueTracking.cpp
134

Just a idea to make sure
How about just set CxtI to nullptr when UseInstrInfo is false? @fhahn @nikic

@@ -115,8 +116,8 @@ struct SimplifyQuery {
                 AssumptionCache *AC = nullptr,
                 const Instruction *CXTI = nullptr, bool UseInstrInfo = true,
                 bool CanUseUndef = true)
-      : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI), IIQ(UseInstrInfo),
-        CanUseUndef(CanUseUndef) {}
+      : DL(DL), TLI(TLI), DT(DT), AC(AC), IIQ(UseInstrInfo),
+        CxtI(UseInstrInfo ? CxtI : nullptr), CanUseUndef(CanUseUndef) {}
   Query(const DataLayout &DL, AssumptionCache *AC, const Instruction *CxtI,
         const DominatorTree *DT, bool UseInstrInfo,
         OptimizationRemarkEmitter *ORE = nullptr)
-      : DL(DL), AC(AC), CxtI(CxtI), DT(DT), ORE(ORE), IIQ(UseInstrInfo) {}
+      : DL(DL), AC(AC), IIQ(UseInstrInfo), CxtI(UseInstrInfo ? CxtI : nullptr),
+        DT(DT), ORE(ORE) {}