This is an archive of the discontinued LLVM Phabricator instance.

[PseudoSourceValue] New category to represent floating-point status
AbandonedPublic

Authored by uweigand on Oct 2 2018, 9:55 AM.

Details

Summary

As described in https://reviews.llvm.org/D45576, this patch creates a new PseudoSourceValue category intended to represent the target floating-point status, specifically the changes to global state that may be *caused* by the instruction, e.g. setting exception status bits or triggering a trap.

The idea is to model these aspects of global floating-point state as "memory" of a special type, even if it may be held in a special register on actual hardware, in order to avoid having to introduce a new type of dependency throughout LLVM. Modeling as memory object should be OK to get the dependencies we need correct.

This class will be used with memory operands attached to strict floating-point instructions at the DAG and MI level.

Patch split off from D45576 to simplify review, but this patch on its own isn't really useful and will only be committed together with D45576 (assuming both are approved).

Diff Detail

Event Timeline

uweigand created this revision.Oct 2 2018, 9:55 AM
kpn added a subscriber: kpn.Oct 2 2018, 10:09 AM
arsenm added a comment.Oct 2 2018, 5:00 PM

How is this going to work? I don't think this can work unless we can start guaranteeing that MMOs are never dropped. Most FP instructions aren't considered as mayLoad or mayStore, and will probably easily lose the MMO if it's the only thing indicating they care about the FP mode

arsenm added a comment.Oct 2 2018, 6:56 PM

How is this going to work? I don't think this can work unless we can start guaranteeing that MMOs are never dropped. Most FP instructions aren't considered as mayLoad or mayStore, and will probably easily lose the MMO if it's the only thing indicating they care about the FP mode

I don't think mayAccessMemory changes the problem. It's still something that won't help verifying the MMO is not dropped

How is this going to work? I don't think this can work unless we can start guaranteeing that MMOs are never dropped. Most FP instructions aren't considered as mayLoad or mayStore, and will probably easily lose the MMO if it's the only thing indicating they care about the FP mode

I haven't seen MMOs being dropped with my current patchset. I believe this should work because:

  • On the SelectionDAG level, we still have the STRICT_... nodes, which mostly aren't touched by optimizers anyway, and when we add some simplification for those nodes, they'll simply have to take care to preserve MMOs. As a sanity check, we could add an assert to verify that at the end of the DAG passes, every STRICT_... node still has a FPStatus MMO.
  • During selection of MIs from DAG nodes, the mayAccessMemory flag *does* help (in fact, this is why I introduced it in the first place ... early versions without that flag did indeed sometimes lose the MMO). This is because of the change to SelectionDAGISel::SelectCodeCommon, which causes instruction patterns marked as mayAccessMemory to inherit MMOs from the selected DAG nodes.
  • On the MI level, once the instruction has the MMO, the MI.mayLoad() and/or MI.mayStore() routines will in fact return true (a mayAccessMemory instruction with MMO is considered equivalent to mayLoad/mayStore). As far as I can see, this should cause the MI-level CodeGen passes to do the right thing (and preserve MMOs).

Am I missing anything here? Where do you think MMOs might still get dropped?

arsenm added a comment.Oct 4 2018, 7:08 AM

How is this going to work? I don't think this can work unless we can start guaranteeing that MMOs are never dropped. Most FP instructions aren't considered as mayLoad or mayStore, and will probably easily lose the MMO if it's the only thing indicating they care about the FP mode

I haven't seen MMOs being dropped with my current patchset. I believe this should work because:

  • On the SelectionDAG level, we still have the STRICT_... nodes, which mostly aren't touched by optimizers anyway, and when we add some simplification for those nodes, they'll simply have to take care to preserve MMOs. As a sanity check, we could add an assert to verify that at the end of the DAG passes, every STRICT_... node still has a FPStatus MMO.
  • During selection of MIs from DAG nodes, the mayAccessMemory flag *does* help (in fact, this is why I introduced it in the first place ... early versions without that flag did indeed sometimes lose the MMO). This is because of the change to SelectionDAGISel::SelectCodeCommon, which causes instruction patterns marked as mayAccessMemory to inherit MMOs from the selected DAG nodes.
  • On the MI level, once the instruction has the MMO, the MI.mayLoad() and/or MI.mayStore() routines will in fact return true (a mayAccessMemory instruction with MMO is considered equivalent to mayLoad/mayStore). As far as I can see, this should cause the MI-level CodeGen passes to do the right thing (and preserve MMOs).

Am I missing anything here? Where do you think MMOs might still get dropped?

I'm not sure where it's documented, but MMOs are definitely not guaranteed to be preserved (except in global-isel, the verifier enforces this for some of the G_* instructions). I'm not aware of anywhere specifically dropping them, but code is not required to preserve them. Instructions possibly accessing memory are then treated as conservatively as possible if they are missing. That doesn't work with this approach. The operand introduces stricter behavior. If something drops it, it now may be incorrectly reordered later, which will be a very subtle bug to detect. My primary concern with this approach (since I guess we could try to adopt a new MMO policy?) is there's no way for the verifier to catch dropping it.

OK, I understand the concern better now, thanks.

I had thought that we'd already need to preserve MMOs, since they contain flags like atomic, nontemporal, or volatile accesses, which likewise would change semantics if dropped.

But I agree it would be good if this could be ensured by the machine verifier.

@hfinkel, you originally suggested use of MMOs --- any comments from your side on this issue?

Now, if we indeed cannot use MMOs to distinguish strict from non-strict FP operations, what could be the alternative? I guess I can see:

  • Different opcodes. That certainly cannot be dropped by accident. But it goes back to each target having to duplicate all FP patterns, which is exactly what we wanted to avoid ...
  • Using MIFlags bits. This may be nice since there's already many FP-related bits in there. However, it seems that there's likewise no way to verify those bits aren't accidentally dropped. (Also, we may run out of bits in the current uint16_t field.)
  • Using a regular MachineOperand, not an MMO. This operand would have to be of some new type, and would have to be inspected by code that currently looks at MMOs to determine dependencies etc. Because this would be in the list of operands of the instruction pattern, its presence would be enforced by the verifier. For non-strict FP instructions, the pattern might include a default no-op value.
  • Making a MMO mandatory. E.g. define that instructions with the mayAccessMemory flag *must* have at least one MMO, which is enforced by the verifier. When initially emitting a non-strict FP MI, CodeGen would create a "dummy" MMO that counts as "no-access" to comply with the rule.

Thoughts?

I'm not sure where it's documented, but MMOs are definitely not guaranteed to be preserved (except in global-isel, the verifier enforces this for some of the G_* instructions). I'm not aware of anywhere specifically dropping them, but code is not required to preserve them.

Why do you say this?

I had thought that we'd already need to preserve MMOs, since they contain flags like atomic, nontemporal, or volatile accesses, which likewise would change semantics if dropped.

Not all of these things are equivalent, as their semantics are assumed to be also part of the instruction. Volatile, however, is not, and that needs to be preserved. I believe that we depend on the fact that MachineInstr::isDereferenceableInvariantLoad returns false for volatile MMOs to prevent MachineCSE from removing some volatile loads.

arsenm added a comment.Oct 4 2018, 8:08 PM

I'm not sure where it's documented, but MMOs are definitely not guaranteed to be preserved (except in global-isel, the verifier enforces this for some of the G_* instructions). I'm not aware of anywhere specifically dropping them, but code is not required to preserve them.

Why do you say this?

Things like MachineInstr::hasOrderedMemoryRef() has handling such as:

// Otherwise, if the instruction has no memory reference information,
// conservatively assume it wasn't preserved.
if (memoperands_empty())
  return true;

I always assumed thing were allowed to drop MMOs to avoid passes having to figure out what to do when an instruction had multiple memoperands.
There is also an explicit MachineInstr::dropMemRefs, which the outliner uses.

I had thought that we'd already need to preserve MMOs, since they contain flags like atomic, nontemporal, or volatile accesses, which likewise would change semantics if dropped.

Not all of these things are equivalent, as their semantics are assumed to be also part of the instruction. Volatile, however, is not, and that needs to be preserved. I believe that we depend on the fact that MachineInstr::isDereferenceableInvariantLoad returns false for volatile MMOs to prevent MachineCSE from removing some volatile loads.

Volatile needs to be preserved if the MMO is present, but if the MMO is missing entirely the operation can be conservatively assumed to be volatile (i.e. the handling in hasOrderedMemoryRef)

I'm not sure where it's documented, but MMOs are definitely not guaranteed to be preserved (except in global-isel, the verifier enforces this for some of the G_* instructions). I'm not aware of anywhere specifically dropping them, but code is not required to preserve them.

Why do you say this?

Things like MachineInstr::hasOrderedMemoryRef() has handling such as:

// Otherwise, if the instruction has no memory reference information,
// conservatively assume it wasn't preserved.
if (memoperands_empty())
  return true;

I always assumed thing were allowed to drop MMOs to avoid passes having to figure out what to do when an instruction had multiple memoperands.

I don't think so. A lot of passes do actively ignore instructions with multiple MMOs, but that always seemed to be the responsibility of the passes in question.

There is also an explicit MachineInstr::dropMemRefs, which the outliner uses.

The outliner is new ;) -- I'm not sure that's the right thing to do.

I had thought that we'd already need to preserve MMOs, since they contain flags like atomic, nontemporal, or volatile accesses, which likewise would change semantics if dropped.

Not all of these things are equivalent, as their semantics are assumed to be also part of the instruction. Volatile, however, is not, and that needs to be preserved. I believe that we depend on the fact that MachineInstr::isDereferenceableInvariantLoad returns false for volatile MMOs to prevent MachineCSE from removing some volatile loads.

Volatile needs to be preserved if the MMO is present, but if the MMO is missing entirely the operation can be conservatively assumed to be volatile (i.e. the handling in hasOrderedMemoryRef)

Interesting. The comment there certainly does seem to imply that. One thing that has always been true is that there have been problems with SDAG code preserving semantic information like that. It's possible that the code is referring more to that than to MMOs being dropped at the MI level itself.

In any case, these aren't exactly like metadata, and it seems like we could mandate that they be preserved.

For floating-point semantics, we might want to change the name too? ;)

As far as I understand MachineMemoryOperands:

  • You are allowed to drop them
  • An operation without MachineMemoryOperand needs to be handled conservatively (i.e. potentially aliases with everything)
  • Matthias

As far as I understand MachineMemoryOperands:

  • You are allowed to drop them

I'm trying to understand the difference between the world where we're allowed to drop these and the world in which we're not. It's not clear what benefit we get from allowing dropping these. There also might be an advantage to dropping the Value*s used for AA (e.g., to better decouple the MI from the IR) which doesn't extend to MMOs in general.

  • An operation without MachineMemoryOperand needs to be handled conservatively (i.e. potentially aliases with everything)

This is certainly true, at least as far as aliasing goes. Regarding volatile semantics it doesn't work.

As I see it, we don't currently drop MMOs in practice (with the exception, now, of the MI outliner?). Doing so breaks, at least, volatile semantics. I think that we should simply document that MMOs can't be dropped in general (although we might drop parts of them, such as AA Value*). Thoughts?

  • Matthias

Ping?

How can we get to a decision on whether or not we can get to a place where we're no longer allowed to drop memoperands?

If we indeed cannot use memoperands for floating-point status, should I try looking for another way to encode this? After looking into the options a bit more, the most natural way might be to add a new MIFlag bit, say FmExcept, that indicates the instruction may raise a floating-point exception flag and/or trap. (Or maybe two bits to distinguish the two cases.) In a first implementation, this bit could be used by MachineInstruction::hasUnmodeledSideEffects to return true if set.

Should I try implementing a solution along those lines?

FWIW, I also think it's important not to drop MachineMemOperands. As Hal points out, MMOs carry non-optional information such as volatile-ness. And the Size is non-optional information at least for the AMDGPU backend.

If the machine outliner does not preserve MMOs, maybe it'd be a good idea to try to understand why that is? And explicitly allow dropping selected parts of them like Hal suggests?

uweigand abandoned this revision.Jun 5 2019, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 3:34 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript