Page MenuHomePhabricator

[DAGCombine] Poison-prove scalarizeExtractedVectorLoad.
ClosedPublic

Authored by fhahn on May 25 2021, 3:53 AM.

Details

Summary

extractelement is poison if the index is out-of-bounds, so just
scalarizing the load may introduce an out-of-bounds load, which is UB.

To avoid introducing new UB, we can mask the index so it only contains
valid indices.

Fixes PR50382.

Diff Detail

Event Timeline

fhahn created this revision.May 25 2021, 3:53 AM
fhahn requested review of this revision.May 25 2021, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 3:53 AM
fhahn updated this revision to Diff 347714.May 25 2021, 9:55 AM
fhahn added a subscriber: jonpa.

Adjust failing SystemZ test. It would be great if someone familiar with SystemZ could take a look to double-check this makes sense. Perhaps @jonpa?

RKSimon added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18425

Is there any chance of NumElts not being a pow2 here?

jonpa added a subscriber: uweigand.May 25 2021, 5:04 PM

Adjust failing SystemZ test. It would be great if someone familiar with SystemZ could take a look to double-check this makes sense. Perhaps @jonpa?

Yes, this looks like a correct test update to me: the changed immediate operand reflects that now only two shifted bits are inserted into %r1 (instead of all 32).

I wonder what the rationale behind this patch is: if the instruction is poison to begin with, then the program is broken and I don't understand how changing the argument value can change that..?

Adding @uweigand as well just in case...

qiucf added a subscriber: qiucf.May 25 2021, 7:56 PM
fhahn added a comment.May 26 2021, 1:18 AM

Adjust failing SystemZ test. It would be great if someone familiar with SystemZ could take a look to double-check this makes sense. Perhaps @jonpa?

Yes, this looks like a correct test update to me: the changed immediate operand reflects that now only two shifted bits are inserted into %r1 (instead of all 32).

I wonder what the rationale behind this patch is: if the instruction is poison to begin with, then the program is broken and I don't understand how changing the argument value can change that..?

Whether the whole program is broken (has UB) depends on how the returned value is used in the callers. Returning poison is not UB, unless the function has the noundef attribute.

Adjust failing SystemZ test. It would be great if someone familiar with SystemZ could take a look to double-check this makes sense. Perhaps @jonpa?

Yes, this looks like a correct test update to me: the changed immediate operand reflects that now only two shifted bits are inserted into %r1 (instead of all 32).

I wonder what the rationale behind this patch is: if the instruction is poison to begin with, then the program is broken and I don't understand how changing the argument value can change that..?

Adding @uweigand as well just in case...

This looks correct to me as well, it simply implements the mask to valid index (which can be merged into the existing risbgn instruction).

efriedma added inline comments.May 27 2021, 12:37 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18425

I can't think of anything that would enforce it being a power of two. The usage of getKnownMinValue() here is also wrong: for a scalable vector, we need to clamp the index relative to the actual number of lanes at runtime, not the minimum number of lanes.

TargetLowering::getVectorElementPointer() has all the logic to do this correctly; can we use it here?

fhahn updated this revision to Diff 348502.May 28 2021, 5:45 AM

Updated to use TLI.getVectorElementPointer, thanks!

fhahn added inline comments.May 28 2021, 5:47 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
18425

Yes, it looks like we can still have non-power-of-2 vectors here. We bail out earlier for scalable vectors.

I adjusted the code to use TargetLowering::getVectorElementPointer() as suggested by Eli! Seems like a great way to simplify the existing logic which I was not aware of, thanks!

This revision is now accepted and ready to land.May 28 2021, 12:35 PM
This revision was landed with ongoing or failed builds.May 30 2021, 3:56 AM
This revision was automatically updated to reflect the committed changes.