Page MenuHomePhabricator

[Instruction] Introduce a predicate mustOperandBeConstant()
Needs ReviewPublic

Authored by jmolloy on Aug 23 2016, 5:47 AM.

Details

Reviewers
spatel
majnemer
Summary

Some operands to instructions may not be variables. Examples are shufflevector masks,
GEP indices when the type is a struct and extractvalue indices.

As this blacklist is difficult to get right, promote it from a SimplifyCFG helper
function into Instruction proper.

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy updated this revision to Diff 68974.Aug 23 2016, 5:47 AM
jmolloy retitled this revision from to [Instruction] Introduce a predicate mustOperandBeConstant().
jmolloy updated this object.
jmolloy added reviewers: spatel, majnemer.
jmolloy set the repository for this revision to rL LLVM.
jmolloy added a subscriber: llvm-commits.
spatel added inline comments.Aug 23 2016, 6:54 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1383–1385

Is this check removed intentionally? Ie, we must assume that the instruction may be malformed?

jmolloy added inline comments.Aug 23 2016, 8:04 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1383–1385

I removed it because I felt that having the predicate in Instruction provide possibly different results based on the current operands would be confusing to debug.

Thinking about it though, having a predicate say "this operand must be constant" when it is currently a variable (because the predicate is being conservative) probably isn't ideal.

I'm conflicted about the right way to go here.

spatel added inline comments.Aug 23 2016, 8:52 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1383–1385

The difference comes into play with intrinsics. And that's one of the motivating cases in SimplifyCFG, right?

Without the extra check, we'll say that all intrinsics require all constant argument operands, and so we won't be able to sink them using phis. If we add the check and see that the intrinsic operand is already a variable, then we may still be able to do the sinking transform on the intrinsic.

In order to keep this patch 'NFC', I'd keep the check. If we decide to change the behavior, I think it should be another patch.

Side note on intrinsic operand requirements: it seems the only place that a constant requirement is encoded/enforced is in the Verifier. Ie, it's not part of the td def itself. If that's right, that part of the Verifier should be refactored, so we can use it here and have a single point of truth for intrinsic operand const-ness.

jmolloy updated this revision to Diff 69023.Aug 23 2016, 12:06 PM

Hi Sanjay,

That sounds good to me. This new patch has the const checking factored out of the Verifier and used in both places.

Cheers,

James

spatel edited edge metadata.Aug 23 2016, 1:31 PM

Grabbed the wrong diff? Phab says there were no changes from the earlier rev.

jmolloy updated this revision to Diff 69086.Aug 24 2016, 12:22 AM
jmolloy edited edge metadata.

:( Evening coding never goes well for me. Here's the actual diff.

spatel added inline comments.Aug 24 2016, 6:50 AM
lib/IR/Verifier.cpp
3872–3878

Now that we have a function dedicated to verifying the constant params, shouldn't the corresponding checks in the switch below this be removed?

Either way, I think we have 2 patches in 1 at this point, so it should be split when committing into:

  1. Add IntrinsicInst::mustOperandBeConstant()
  2. Add Instruction::mustOperandBeConstant()

Hi Sanjay,

I agree with splitting the patches prior to commit.

I didn't remove the duplicate asserts because I thought it'd be very strange coding style to downcast and then not check the result of that downcast in a verifier, even if that is also checked elsewhere. I'm open to input on this though.

James

spatel added inline comments.Aug 24 2016, 8:30 AM
lib/IR/Verifier.cpp
3872–3877

Hmm...I'm not sure if I'm following the downcast argument. When we reach this code, we've verified that the function signature and name are ok.

  1. Can we assert that this line:
if (auto *II = dyn_cast<IntrinsicInst>(IF))

cannot fail? Ie, Assert(isa<IntrinsicInst>(IF)).

  1. Should we also assert that
II.getIntrinsicID() == ID

after #1?

I think that would ensure that the subsequent switch const arg checks are redundant (and therefore could be removed), or do you see some other way that we might fall through?

majnemer edited edge metadata.Aug 24 2016, 9:18 AM

There are more specific constraints for some intrinsics. For example, stackprotector must not have a phi or select as its operand: it must be an AllocaInst.

I wonder if the right thing to do here is have a different function (somewhere) which determines if an llvm::Value * is an appropriate operand for some llvm::User *.

lib/IR/Instruction.cpp
499–500

This will not work, Invokes are not descendants of IntrinsicInst.

lib/IR/IntrinsicInst.cpp
88–89

This seems incomplete (it is missing at least prefetch and coro_id).

lib/IR/Verifier.cpp
3872–3877

An IntrinsicInst is a descendant of CallInst but the CallSite might be an InvokeInst of an intrinsic function. IIUC, this would make it possible for the dyn_cast to fail.

Hi David,

Thanks for your comments. I'm a little wary of creating an all-singing-all-dancing version of this predicate for the same reason I don't like this patch very much - duplicating logic in the Verifier. It seems to me that such a predicate "isValueAppropriateForUser()" would replicate a lot of the Verifier logic. The Verifier would need to keep its own logic in place too as it does more stringent checks, for example recursing into dbgIntrinsics and validating metadata.

A very heavy hammer would possibly be to change the verifier to, instead of asserting on a failure, call a callback so we could call it as a predicate. But the verifier isn't really written for speed or to be used in production code.

My rationale with this patch was that, by keeping the scope of the predicate small, I could avoid duplicating as much of the verifier code as possible. I'd appreciate any advice here.

Cheers,

James

spatel added a comment.Jul 1 2019, 9:02 AM

Abandon?
D57825 created an alternate way to specify const-ness with intrinsics.
D58233 would extend that.

Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 9:02 AM