[PATCH] D52785: [PseudoSourceValue] New category to represent floating-point status

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D52785#1256228, @arsenm wrote:

> In https://reviews.llvm.org/D52785#1256226, @hfinkel wrote:
>
> > > 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? ;)


Repository:
  rL LLVM

https://reviews.llvm.org/D52785