Page MenuHomePhabricator

[CSInfo] Do not create CSInfo for undef arguments
ClosedPublic

Authored by ntesic on Dec 2 2020, 2:12 AM.

Details

Summary

If a function parameter is marked as "undef", prevent creation
of CallSiteInfo for that parameter.
Without this patch, the parameter's call_site_value would be incorrect.
The incorrect call_value case reported in PR39716,
addressed in D85111.

Diff Detail

Event Timeline

ntesic created this revision.Dec 2 2020, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 2:12 AM
ntesic requested review of this revision.Dec 2 2020, 2:12 AM
djtodoro added a comment.EditedDec 2 2020, 4:36 AM

This makes sense to me. The call site info represents info about param value transferring, so if the value was undef, we have "nothing to transfer".

dstenb added a comment.Dec 2 2020, 7:23 AM

I think it would be preferable if we could do this in a target independent place, so that downstream targets, and upstream targets that do not yet support call sites, do not have to care about this.

Can we do this in collectCallSiteParameters by looking at the call instruction's undef uses?

Something like this:

@@ -787,6 +787,15 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     (void)InsertedReg;
   }
 
+  for (auto &MO : CallMI->uses()) {
+    if (!MO.isReg() || !MO.isUndef())
+      continue;
+    auto It = ForwardedRegWorklist.find(MO.getReg());
+    if (It == ForwardedRegWorklist.end())
+      continue;
+    ForwardedRegWorklist.erase(It);
+  }
+

I think it would be preferable if we could do this in a target independent place, so that downstream targets, and upstream targets that do not yet support call sites, do not have to care about this.

Can we do this in collectCallSiteParameters by looking at the call instruction's undef uses?

Something like this:

@@ -787,6 +787,15 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     (void)InsertedReg;
   }
 
+  for (auto &MO : CallMI->uses()) {
+    if (!MO.isReg() || !MO.isUndef())
+      continue;
+    auto It = ForwardedRegWorklist.find(MO.getReg());
+    if (It == ForwardedRegWorklist.end())
+      continue;
+    ForwardedRegWorklist.erase(It);
+  }
+

It makes sense to me.

ntesic added a comment.Dec 3 2020, 4:29 AM

I think it would be preferable if we could do this in a target independent place, so that downstream targets, and upstream targets that do not yet support call sites, do not have to care about this.

Can we do this in collectCallSiteParameters by looking at the call instruction's undef uses?

Something like this:

@@ -787,6 +787,15 @@ static void collectCallSiteParameters(const MachineInstr *CallMI,
     (void)InsertedReg;
   }
 
+  for (auto &MO : CallMI->uses()) {
+    if (!MO.isReg() || !MO.isUndef())
+      continue;
+    auto It = ForwardedRegWorklist.find(MO.getReg());
+    if (It == ForwardedRegWorklist.end())
+      continue;
+    ForwardedRegWorklist.erase(It);
+  }
+

Thanks, I agree with your proposal.

ntesic updated this revision to Diff 309226.Dec 3 2020, 4:31 AM
ntesic retitled this revision from [CSInfo][ISEL] Do not create CSInfo for undef arguments to [CSInfo] Do not create CSInfo for undef arguments.
ntesic edited the summary of this revision. (Show Details)
  • Move implementation to DwarfDebug
  • Update tests to check DWARF
dstenb accepted this revision.Dec 7 2020, 2:50 AM

LGTM.

As both the emission of the IMPLICIT_DEF instructions in SelectionDAG, and the resolving of those instructions in "Process Implicit Definitions", is target independent code, I think it would be sufficient with only keeping one of these test cases, but I would be fine with landed this with all five.

This revision is now accepted and ready to land.Dec 7 2020, 2:50 AM
djtodoro accepted this revision.Dec 7 2020, 3:26 AM

lgtm, but please keep just one test case, since it is enough (as @dstenb suggested)

ntesic added a comment.Dec 9 2020, 2:54 AM

@dstenb @djtodoro Sure, I will keep only one test.
Thanks for the review!